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

Set CMake flags in post_import phase #99

Merged
merged 1 commit into from
Aug 31, 2016

Conversation

chhtz
Copy link
Contributor

@chhtz chhtz commented Aug 30, 2016

It seems that pkg.tags is not set at the moment Rock.update_cmake_build_type_from_tags(pkg) is called.
I reverted this part to a cleaned up version before #92

@jmachowinski
Copy link
Contributor

I can confirm this, only works if set in post import

@doudou
Copy link
Member

doudou commented Aug 30, 2016

@chhtz: are you running autoproj v1 or v2 ? Could you try defining it inside a setup_package block instead ?, e.g.

   when Autobuild::CMake
        setup_package(pkg) do
           Rock.update_cmake_build_type_from_tags(pkg)
        end
        pkg.define "ROCK_TEST_ENABLED", pkg.test_utility.enabled?
        pkg.define "CMAKE_EXPORT_COMPILE_COMMANDS", "ON"

Note also that the defines should stay where they are unless there also is a problem with them in that block.

@chhtz
Copy link
Contributor Author

chhtz commented Aug 30, 2016

I'm on autoproj v1.13.5.
With the setup_package I get:

#<Autobuild::CMake:0x000000013a4458> is not a known package
  in /home/chtz/workspace/entern/autoproj/remotes/rock.core/overrides.rb:73:in `block in <top (required)>'
  in /home/chtz/workspace/entern/autoproj/remotes/rock.core/overrides.rb:35:in `<top (required)>'

Removing the setup_package (i.e., reverting to without my PR) sets all my packages to DEBUG

@doudou
Copy link
Member

doudou commented Aug 30, 2016

Sorry, on v1 it should be setup_package(pkg.name) do

@chhtz
Copy link
Contributor Author

chhtz commented Aug 30, 2016

With that it switches everything to Debug again:

$ amake base/types/
[...]
autoproj: importing and loading selected packages
  base/types: changed value of CMAKE_BUILD_TYPE from Release to Debug
  base/cmake: changed value of CMAKE_BUILD_TYPE from Release to Debug
[...]

@chhtz
Copy link
Contributor Author

chhtz commented Aug 30, 2016

N.B.: I'm not sure about the ROCK_TEST_ENABLED, CMAKE_EXPORT_COMPILE_COMMANDS
At the moment it seems to work in either place (I thought I had an issue with one of them as well).

@doudou
Copy link
Member

doudou commented Aug 30, 2016

The issue is related to the order between loading of manifest.xml (the tags) and the call of setup_package. The defines are not dependent on files in the package, so are not subject to the same constraints. I just read the whole file, and it seems that it is the only one that has this problem.

Anyways, last one: could you try just doing

pkg.post_import do
   ... handle tags ...
end

at the original place, instead of moving it to a different place in the file ? It would help readability. Thanks a lot for your help.

@chhtz
Copy link
Contributor Author

chhtz commented Aug 30, 2016

Seems to work as well. I overwrote the commit and only changed handling of the build type now.

@doudou
Copy link
Member

doudou commented Aug 30, 2016

Mmm ... miscommunication.

Could you try leaving the tags stuff in the case ... when ... end block, but surrounding it with the (arguably better)

pkg.post_import do
    Rock.update_cmake_build_type_from_tags(pkg)
end

?

@chhtz
Copy link
Contributor Author

chhtz commented Aug 31, 2016

Sorry for the delay and the misunderstanding. I updated the PR (meaning this variant works as well).

@doudou doudou merged commit 3918759 into rock-core:master Aug 31, 2016
@chhtz chhtz deleted the post_import_cmake_flags branch August 31, 2016 11:42
pierrewillenbrockdfki pushed a commit to pierrewillenbrockdfki/rock_core_package_set that referenced this pull request Feb 2, 2023
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.

None yet

3 participants