[SERVER-79573] add IWYU export pragmas to unittest.h Created: 01/Aug/23  Updated: 16/Oct/23

Status: Backlog
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Billy Donahue Assignee: Backlog - Service Architecture
Resolution: Unresolved Votes: 0
Labels: techdebt
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Problem/Incident
is caused by SERVER-77049 IWYU auto changes for compiledb 0.8 -... Closed
Assigned Teams:
Service Arch
Participants:

 Description   

I just noticed a test (itoa_test.cpp in this case) was modified from its include of

#include "mongo/unittest/unittest.h"

to now use the granular includes of the components of the unittest library:

#include "mongo/unittest/assert.h"
#include "mongo/unittest/framework.h"

This is a downgrade, as the unittest.h header is designed as an umbrella header for the API.
These smaller more granular headers are basically implementation details designed to break dependency cycles when necessary.
But unit tests without special dependency considerations should be using the unittest/unittest.h header and not the subcomponents.

The unit tests affected by this migration should have their includes of unittest/unittest.h restored.

There's an IWYU pragma that can be added to unittest.h to prevent future IWYU passes from doing this again.

https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md#iwyu-pragma-begin_exportsend_exports

We'd make unittest/unittest.h look like this:

#pragma once
// IWYU pragma: begin_exports
#include "mongo/unittest/assert.h"
#include "mongo/unittest/assert_that.h"
#include "mongo/unittest/bson_test_util.h"
#include "mongo/unittest/framework.h"
// IWYU pragma: end_exports



 Comments   
Comment by Billy Donahue [ 15/Aug/23 ]

Optionally undo the IWYU changes that broke up the includes of the umbrella header.
This might be elegantly scriptable.

Comment by Steve Gross [ 10/Aug/23 ]

billy.donahue@mongodb.com SGTM; I see the issue is assigned to service arch.

Comment by Steve Gross [ 10/Aug/23 ]

Per conversation w/ billy.donahue@mongodb.com :

  • I confirmed that the IWYU rollout was–for now–a 1-time event; that is, we do not have a bot that will re-execute IWYU refactors.
  • With this in mind, billy.donahue@mongodb.com (or any engineer, for that matter), is free to change around #includes (including "umbrella" includes) as they see fit.
  • We hope to reevaluate if/how to use IWYU on an ongoing basis once daniel.moody@mongodb.com is back in the office (October-ish).
  • Thanks for everyone's patience on this; I realize that IWYU has far-reaching effects on engineers' daily lives.
Comment by Jason Chan [ 09/Aug/23 ]

steve.gross@mongodb.com, do we have an idea how real/significant the build penalty would be? Since Service Arch owns the unit test infrastructure, this decision directly impacts how that component is maintained moving forward.

Comment by Billy Donahue [ 09/Aug/23 ]

Sure. Let's have that talk.

Comment by Steve Gross [ 08/Aug/23 ]

billy.donahue@mongodb.com You make an interesting point. The use of "umbrella" includes is an interesting technique, and can be useful in some cases. The tradeoff, however, is possible inefficient/slower builds (hence the IWYU refactor to eliminate umbrella includes). It seems like a broader engineering discussion to have, however; FWIW, my hot take is that umbrellas aren't worth the build penalty. With that said, let's chat more about this; maybe start a slack thread to discuss in more detail?

Generated at Thu Feb 08 06:41:19 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.