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 check for ROS2 distribution in CMakeLists.txt #345

Merged

Conversation

arturkamieniecki
Copy link
Contributor

I've modified CMakeLists.txt to check if the initial configuration was run under the same ROS 2 distribution as the one sourced during project setup and build. I've changed CMakeLists.txt such that the ROS 2 distribution will be cached into CMakeCache.txt during the project's configuration. A check during the configuration process prevents users from building when their sourced distribution is different. I've also added the same check during the build process.

Removing the CMakeCache.txt, as proposed by me (Remove CMakeCache.txt and reconfigure project.), does not cause rebuilding of the O3DE Engine.

This addresses #135.

Signed-off-by: Artur Kamieniecki <artur.kamieniecki@robotec.ai>
@arturkamieniecki arturkamieniecki requested review from a team as code owners May 22, 2023 15:47
Copy link
Contributor

@nick-l-o3de nick-l-o3de left a comment

Choose a reason for hiding this comment

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

looks like it will work - is it necessary to remove the cache, though?
Does it cache stuff that it doesn't recompute even if you do a reconfigure?

To be more specific, do we have to force a cache deletion here, or can we force an auto-reconfigure, since a lot of stuff is checked in the configuration step but a problem is that env vars changing doesn't cause a reconfigure...

@lemonade-dm
Copy link
Contributor

looks like it will work - is it necessary to remove the cache, though? Does it cache stuff that it doesn't recompute even if you do a reconfigure?

To be more specific, do we have to force a cache deletion here, or can we force an auto-reconfigure, since a lot of stuff is checked in the configuration step but a problem is that env vars changing doesn't cause a reconfigure...

An alternative is using the -UROS_DISTRO_TYPE option to undefine the ROS_DISTRO_TYPE cache variable to force a new check of the ROS2 distro

@nick-l-o3de
Copy link
Contributor

nick-l-o3de commented May 22, 2023

I'm thinking more along the lines of what inside the cache is never checked again even on reconfigure. An example is things like when find_program or find_library is used, unless those vars are cleared, or FORCE is used, cmake tends to cache them. I ran into this quite a bit when dealing with the 3p system. To actually force those you'd have to also have to undef (in the cache) the vars that are caching those program locations.

@lemonade-dm
Copy link
Contributor

lemonade-dm commented May 22, 2023

I'm thinking more along the lines of what inside the cache is never checked again even on reconfigure. An example is things like when find_program or find_library is used, unless those vars are cleared, or FORCE is used, cmake tends to cache them. I ran into this quite a bit when dealing with the 3p system. To actually force those you'd have to also have to undef (in the cache) the vars that are caching those program locations.

How about checking if the value in the Environment variable is different than the one in the cache if the cache variable is set?
If so force update the cache with the new value.

That can be an option instead of a message(FATAL_ERROR)

@nick-l-o3de
Copy link
Contributor

I'm thinking more along the lines of what inside the cache is never checked again even on reconfigure. An example is things like when find_program or find_library is used, unless those vars are cleared, or FORCE is used, cmake tends to cache them. I ran into this quite a bit when dealing with the 3p system. To actually force those you'd have to also have to undef (in the cache) the vars that are caching those program locations.

How about checking if the value in the Environment variable is different than the one in the cache if the cache variable is set? If so force update the cache with the new value.

That can be an option instead of a message(FATAL_ERROR)

I think the one where it fails the build due to an error in the build step is valid, since CMake doesn't actually check the value of Env Vars by default during build if nothing has changed in its dependency tree.

So maybe the flow would be

If you configure, it wipes the vars and configures with the new sources / env vars.
If you build without configuring and it skips the confiugure step, the build step detects the mismatch, and forces a failure but also clears the cache variables or touches a file or something that wil cause the next attempt to auto-reconfigure?

@lemonade-dm
Copy link
Contributor

lemonade-dm commented May 22, 2023

I'm thinking more along the lines of what inside the cache is never checked again even on reconfigure. An example is things like when find_program or find_library is used, unless those vars are cleared, or FORCE is used, cmake tends to cache them. I ran into this quite a bit when dealing with the 3p system. To actually force those you'd have to also have to undef (in the cache) the vars that are caching those program locations.

How about checking if the value in the Environment variable is different than the one in the cache if the cache variable is set? If so force update the cache with the new value.
That can be an option instead of a message(FATAL_ERROR)

I think the one where it fails the build due to an error in the build step is valid, since CMake doesn't actually check the value of Env Vars by default during build if nothing has changed in its dependency tree.

So maybe the flow would be

If you configure, it wipes the vars and configures with the new sources / env vars. If you build without configuring and it skips the configure step, the build step detects the mismatch, and forces a failure but also clears the cache variables or touches a file or something that will cause the next attempt to auto-reconfigure?

Ok, that can work 95% of the way. The one thing that can't be done is to clear the cache from the build step since checkROS2Distribution.cmake custom target doesn't have access to the cmake binary directory cache values since it is an external command.

@nick-l-o3de
Copy link
Contributor

I think the one where it fails the build due to an error in the build step is valid, since CMake doesn't actually check the value of Env Vars by default during build if nothing has changed in its dependency tree.
So maybe the flow would be
If you configure, it wipes the vars and configures with the new sources / env vars. If you build without configuring and it skips the configure step, the build step detects the mismatch, and forces a failure but also clears the cache variables or touches a file or something that will cause the next attempt to auto-reconfigure?

Ok, that can work 95% of the way. The one thing that can't be done is to clear the cache from the build step since checkROS2Distribution.cmake custom target doesn't have access to the cmake binary directory cache values since it is an external command.

(Ugh, 'edit' is not 'reply, sorry)

Yeah. maybe if the build command simply detects the problem and "does something" to cause the next time to reconfigure (such as touch a dependency file, before exiting with failure), then configure can check the new cache and if it doesn't match the old value, unset a bunch of vars to force find_program, find_library, et al to recompute...

The given PR is not a terrible "stop gap measure" though.

Signed-off-by: Artur Kamieniecki <artur.kamieniecki@robotec.ai>
@arturkamieniecki
Copy link
Contributor Author

I've added the suggestion and the "nuclear option" as lawsonamzn called it.

Copy link
Contributor

@nick-l-o3de nick-l-o3de left a comment

Choose a reason for hiding this comment

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

Nice. Its a brute force method but from the user's point of view they will get the error, and then if they try again, cmake will reconfigure and retry automatically...

Copy link
Contributor

@michalpelka michalpelka left a comment

Choose a reason for hiding this comment

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

💥

Copy link
Contributor

@adamdbrw adamdbrw left a comment

Choose a reason for hiding this comment

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

Nice work!

@nick-l-o3de
Copy link
Contributor

I did a clean build, it looks like it passed.

@michalpelka michalpelka merged commit 2780702 into o3de:development Jun 1, 2023
2 checks passed
arturkamieniecki added a commit to arturkamieniecki/o3de-extras that referenced this pull request Jul 5, 2023
* Add check for ROS2 distribution in CMakeLists.txt
* Add suggestions and automatic removal of invalid cache

Signed-off-by: Artur Kamieniecki <artur.kamieniecki@robotec.ai>

---------

Signed-off-by: Artur Kamieniecki <artur.kamieniecki@robotec.ai>
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

6 participants