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

Refactor celix_container_bundles_dir to use add_custom_target #594

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

pnoltes
Copy link
Contributor

@pnoltes pnoltes commented Jul 25, 2023

This PR addresses an issue where changes to bundle zip files were not being copied to the "bundles" directory in the container.

The process of copying updated bundles to a container "bundles" dir appears to have broken at some point. Unfortunately, I couldn't trace back the specific change that introduced this issue using git blame.

Originally, the copying of bundles was tracked with a timestamp file, in combination with a CMake add_custom_command. However, attempts to resolve the problem with the add_custom_command proved unsuccessful, leading me to replace it with add_custom_target.

In my opinion, a custom target is less than ideal, as a custom target dependency always triggers whenever a target is built. A comparison of the time to run make -j when nothing has changed demonstrated that on my systems, it took twice as long (albeit still under 1 second).

An upside is that the current change seems to fix the usage CMake -> Ninja, which I previously had issues with.
Building with ninja, compared to make -j, is significantly faster.

In conclusion, despite its drawbacks, I believe this update is necessary for now to resolve the issue. Possibly in the future, we can revert to using add_custom_command instead of add_custom_target.

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2023

Codecov Report

Merging #594 (aece685) into master (456de19) will decrease coverage by 0.52%.
The diff coverage is n/a.

❗ Current head aece685 differs from pull request most recent head 3b55bfb. Consider uploading reports for the commit 3b55bfb to get more accurate results

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
- Coverage   79.00%   78.48%   -0.52%     
==========================================
  Files         234      234              
  Lines       35386    35386              
==========================================
- Hits        27955    27772     -183     
- Misses       7431     7614     +183     

see 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@PengZheng
Copy link
Contributor

PengZheng commented Jul 26, 2023

Originally, the copying of bundles was tracked with a timestamp file, in combination with a CMake add_custom_command. However, attempts to resolve the problem with the add_custom_command proved unsuccessful, leading me to replace it with add_custom_target.

We have already done this timestamp thing several times successfully in BundlePackaging.cmake.
I am curious why it didn't work this time.

Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

LGTM

It's nice to support ninja build and to have container use latest bundles.
We can investigate further optimization later.
An issue is created for this optimization.

Note that Conan use Unix Makefile by default.
One non-intrusive way (without changing conanfile.py) of using ninja is to set environment variable CONAN_CMAKE_GENERATOR=Ninja.

@pnoltes
Copy link
Contributor Author

pnoltes commented Jul 26, 2023

We have already done this timestamp thing several times successfully in BundlePackaging.cmake.
I am curious why it didn't work this time.

I agree, I would like to test if this broke with a newer cmake. But for now a fix with custom targets is IMO good enough.

@pnoltes pnoltes merged commit 8bcac0f into master Jul 26, 2023
@pnoltes pnoltes deleted the hotfix/container_bundle_copies_do_not_trigger branch July 26, 2023 17:23
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.

3 participants