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

Use BUILD_INTERFACE generator expression to fix cmake flag exports #3485

Merged
merged 5 commits into from Oct 12, 2018

Conversation

Projects
None yet
3 participants
@biddisco
Copy link
Contributor

commented Oct 9, 2018

Fixes #3484

Proposed Changes

Use BUILD_INTERFACE generator expression to make flags only inherited by targets n the build tree

@sithhell

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

I think that's not sufficient. I have a different solution here:
https://gist.github.com/sithhell/668c7f76f474980df00abde9af98ac00

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

The BUILD_INTERFACE works fine. Can you explain what your patch does? I would like to move away from all the monolithic hpx_add_xxx macros that should not be necessary and instead migrate towards just target_add_xxx at the CMakeLists level.

@sithhell

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

what it does is to declare certain compiler flags private and others public. As you wrote in #3484, your patch only works for an installation of HPX. When running off of a build directory, the problem remains and a lot of applications are affected by the warnings (and warnings as errors) that now turn up.

I agree with you that moving away from those properties should be the way to go. In the meantime, we should at least restore the behavior we had for the release (which my linked patch does).

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

OK. understood. yes adding PUBLIC and PRIVATE options to the flags is a good idea - in the next step we can then just get rid of the macro altogether :) (when cleanup is done)

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Regarding your point that BUILD_INTERFACE isn't enough, and adding PUBLIC/PRIVATE attributes - if a flag is marked as PRIVATE then the tests will not pick up that flag. If it is PUBLIC they will, but so will users' code. Using BUILD_INTERFACE allows anything to be used in the project and it is only users building against the build dir will be affected. BUILD_INTERFACE solves the problem entirely for installed versions of HPX. Using PRIVATE will mean some tests are not getting all the flags. A combination of both would be a good idea.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

@biddisco when will the issue (#3484) be fixed? If you anticipate for the fix to take a while I'd suggest rolling back the cmake changes committed by #3479.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

@sithhell could you please fix this? We should get this PR in as quickly as possible. Alternatively, I suggest to go ahead and roll back #3479.

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

@hkaiser I will fix it. Got sidetracked by the other PR.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

@biddisco thanks a lot!

biddisco and others added some commits Oct 9, 2018

Add an empty INTERFACE target to pass compile flags to tests
An INTERFACE target can have no source files, and no library
but passes properties on to targets that depend on it.
We add flags such as -Werror=XXX to the hpx_test_flags target
and make all tests depend on it.

The flags are not visible to user code via the build or install dirs
when they link against the main hpx library that has the same flags
set using the PRIVATE option.

@biddisco biddisco force-pushed the fix_3484 branch from de46200 to 87da0dc Oct 10, 2018

@biddisco biddisco force-pushed the fix_3484 branch from 665fe11 to 237f78f Oct 11, 2018

Add compile options/flags to libraries/components and tests
The warning and error flags are only set now, if we are compiling within the
tree. The extra flags are not exported.

@sithhell sithhell force-pushed the fix_3484 branch from 237f78f to e50b55d Oct 11, 2018

@sithhell

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

Everything should be back to normal now. Not sure why the parallel algorithm test is constantly failing now :(

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

Not sure why the parallel algorithm test is constantly failing now

@sithhell this is because circleci has significantly reduced the amount of memory available to the vm's they run the tests on.

@biddisco

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

If phylanx is now working, then we should merge this

@sithhell

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Phylanx is working fine again.

@sithhell sithhell merged commit bb22edc into master Oct 12, 2018

44 of 46 checks passed

pycicle greina-gcc-7.2.0-Boost-1.67.1-Debug Build errors 35
Details
pycicle greina-gcc-7.2.0-Boost-1.67.1-Debug Test errors 3
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: clang_tidy Your tests passed on CircleCI!
Details
ci/circleci: configure Your tests passed on CircleCI!
Details
ci/circleci: core Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: examples Your tests passed on CircleCI!
Details
ci/circleci: inspect Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: tests.examples Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.compat Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.components Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.compute Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.config Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.include Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.lcos Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.parallel Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.performance_counters Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.plugins Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.runtime Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.traits Your tests passed on CircleCI!
Details
ci/circleci: tests.headers.util Your tests passed on CircleCI!
Details
ci/circleci: tests.performance Your tests passed on CircleCI!
Details
ci/circleci: tests.regressions Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.actions Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.agas Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.build Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.component Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.computeapi Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.diagnostics Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.lcos Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.parallel.algorithms Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.parallel.block Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.parallel.container_algorithms Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.parallel.datapar_algorithms Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.parallel.executors Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.parallel.segmented_algorithms Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.parcelset Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.performance_counter Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.resource Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.serialization Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.threads Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.traits Your tests passed on CircleCI!
Details
ci/circleci: tests.unit.util Your tests passed on CircleCI!
Details
pycicle greina-gcc-7.2.0-Boost-1.67.1-Debug Config errors 0
Details

@sithhell sithhell deleted the fix_3484 branch Oct 12, 2018

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@sithhell Does this enforce the C++ mode onto dependent libraries/applications now?

We are seeing the issue, that Phylanx does not compile with Blaze master. This is a combination of things, but it is exposed by us not being able to enforce compilation of Phylanx in C++14 mode (CircleCI) as the HPX base Docker image enforces C++17.

@hkaiser hkaiser restored the fix_3484 branch Oct 12, 2018

hkaiser added a commit that referenced this pull request Oct 12, 2018

@hkaiser hkaiser deleted the fix_3484 branch Oct 12, 2018

@sithhell

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

That's strange. We always enforced the C++ mode to our consuming target.

Ok, then we will need to make sure HPX is compiled in C++14 mode on CircleCI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.