Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for modular build structure. #183

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

grafikrobot
Copy link
Member

@grafikrobot grafikrobot commented Jul 20, 2024

This is part of the effort to make the Boost libraries "modular" for build and consumption. See https://lists.boost.org/Archives/boost/2024/01/255704.php and https://github.com/grafikrobot/boost-b2-modular/blob/b2-modular/README.adoc for more information.

This PR depends on the following other PRs being merged to both develop and master branches of the respective repos:

This PR will be changed to ready for review, i.e. not draft, when the above are merged. Do not merge this one until that time.

@grafikrobot grafikrobot marked this pull request as ready for review August 18, 2024 15:27
@grafikrobot
Copy link
Member Author

Please review and merge this PR at your earliest convenience.

Copy link
Collaborator

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that now the warning settings get propagated to dependencies which cause failures when build libraries that have a looser warning setting than this one.

I.e. I want this library to emit no warnings testing it via -Werror but ignore any warnings from dependencies, especially those in the sources of the dependencies, e.g. now in libs\filesystem\src\operations.cpp, see boostorg/filesystem#321

Can this be done?

The GHA failures should be fixed in develop, no need to worry about those.


feature.feature boost.nowide.lfs : auto no : optional propagated ;

constant boost_dependencies :
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for the variable instead of inlining in build/Jamfile?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason is to advertise the dependencies to others consistently for both buildable and header only libs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that's intended for ("others"=)humans, isn't it? Because otherwise I don't see how this is consistent with e.g. https://github.com/boostorg/variant2/blob/a4b167b7238781d75df5d6684c9e9d40b26456dc/build.jam#L17 given that are different files

My hesitation is because now when reading/editing the build/Jamfile I need to switch to this to see the definition of a variable used there. For head-only libs it is defined and used in the same file (as the build-Jamfile doesn't exist)

To be clear: I'm not saying this is bad, I just want to understand the motivation why.

@Flamefire
Copy link
Collaborator

Flamefire commented Aug 27, 2024

@grafikrobot Anything new about not "up-propagating" warning settings? See https://github.com/boostorg/nowide/actions/runs/10547775762/job/29220866970?pr=183

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.45%. Comparing base (43f53ec) to head (756e3dc).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #183   +/-   ##
========================================
  Coverage    99.45%   99.45%           
========================================
  Files           34       34           
  Lines         3319     3319           
========================================
  Hits          3301     3301           
  Misses          18       18           

@Flamefire
Copy link
Collaborator

@grafikrobot See https://github.com/boostorg/nowide/pull/183/files#r1730327788

Why is boost_dependencies a constant and not just inlined in the single place it is used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants