apache / celix

Apache Celix is a framework for C and C++14 to develop dynamic modular software applications using component and in-process service-oriented programming.
https://celix.apache.org/
Apache License 2.0
162 stars 86 forks source link

Refactor celix_container_bundles_dir to use add_custom_target #594

Closed pnoltes closed 1 year ago

pnoltes commented 1 year ago

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 commented 1 year ago

Codecov Report

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

:exclamation: 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

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

PengZheng commented 1 year ago

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.

pnoltes commented 1 year ago

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.