Open-CMSIS-Pack / cbuild

Commandline utility to orchestrate the intermediate build steps of CMSIS Build
Apache License 2.0
3 stars 10 forks source link

Commit d74fe33 breaks project post-build execute steps #244

Closed EaselinkBachmann closed 2 months ago

EaselinkBachmann commented 3 months ago

Since commit d74fe33 (Run cbuild2cmake per context), post-build execute steps in projects are no longer executed. Pre-build steps that create files needed by the build are still executed.

This is because all execute steps are created as separate targets in the "superlists" CMakeLists.txt, but cbuild now invokes only the project+context target, which misses the post-build step targets.

Reverting commits 6059425 and d74fe33 fixes the issue when building all contexts (default behaviour of cbuild). However, compiling single projects with --context does not run post-build steps (and also did not run post-build steps before the commit that broke this).

Expected Behaviour

Actual Behaviour

Example Executes Step

    - execute: Generating application hexdump
      run: xxd $input$ $output$
      input:
        - $bin()$
      output:
        - $OutDir()$/application_hexdump.txt

Potential Fixes

The most reliable fix is probably to create a separate CMake target that has dependencies on the main build step and all pre- and post-build steps for a project, and have cbuild invoke those targets instead of just the main build step target.

Since execute steps are not associated with their corresponding projects at the moment in the .cbuild-idx.yml (the project/context name is only available as part of the execute string) this might need a bit of restructuring to implement properly.

brondani commented 3 months ago

@EaselinkBachmann Thanks for reporting this. Calling e.g. cmake --build tmp --target all or omitting the --target option after building the selected contexts should fix the regression concerning the build of the whole solution (or context-set). We should provide a PR soon.

As it is implemented today an execute node is not defined as pre- or post-build step, the point in time when it is triggered depends only on declared inputs and outputs. Specifying a context triggers the build target of such context and its dependencies, but not other targets that depend on it. In this new scenario a specific post-build execute could/should be called with the -t option, for example cbuild <solution>.csolution.yml -t <my_post-build_step>, which would trigger all dependencies needed to generate the output of such execute.

In the Open-CMSIS-Pack meeting from 2024-05-07 at the minute 41:00 I started demoing the executes nodes and at about 53:50 I spoke about the calling "post-build steps" in this new concept.

EaselinkBachmann commented 3 months ago

I can confirm that building the all target of the generated CMake project works and understand that execute nodes are currently only implicitly added to the build command.

Manually calling the CMake target works, but requires knowing the exact absolute path of the output file for execute nodes with outputs, and requires using the mangled target name (project+context-Executes_description) for execute nodes without explicit outputs. It also makes it really inconvenient to manually trigger multiple independent post build steps for a project if building all other projects is not intended.

IMO the description of execute nodes on projects in the spec implies that execute nodes are automatically run as part of building a project/context, as nothing to the contrary is specifically stated.

Calling e.g. cmake --build tmp --target all or omitting the --target option after building the selected contexts should fix the regression concerning the build of the whole solution (or context-set). We should provide a PR soon.

This works, but would potentially trigger the whole project to be built twice if one of the pre-build steps is an execute node with the always: flag, though that might be unavoidable given the CMake structure.

Specifying a context triggers the build target of such context and its dependencies, but not other targets that depend on it.

This seems reasonable from an implementation strategy point of view, but is unexpected for users given the structure of the project yaml files (a user might expect execute nodes from the project yaml file to be run when that project is built).

In the Open-CMSIS-Pack meeting from 2024-05-07 at the minute 41:00 I started demoing the executes nodes and at about 53:50 I spoke about the calling "post-build steps" in this new concept.

Thank you for the resource, I will look into it.

brondani commented 2 months ago

The regression was fixed in https://github.com/Open-CMSIS-Pack/cbuild/pull/256 and will be part of the upcoming CMSIS-Toolbox release 2.5.0.

Triggering "post-build" executes described at project level as part of a context build should be possible when generating them in CMakeLists using ExternalProject_Add_Step for the related contexts instead of custom target/command. It should be scheduled for a future release.

EaselinkBachmann commented 2 months ago

The regression was fixed in #256 and will be part of the upcoming CMSIS-Toolbox release 2.5.0.

Thanks!

Triggering "post-build" executes described at project level as part of a context build should be possible when generating them in CMakeLists using ExternalProject_Add_Step for the related contexts instead of custom target/command. It should be scheduled for a future release.

Yes, from local testing I can confirm this works. It also works to add an empty ExternalProject_Add_Step(${CONTEXT} executes DEPENDEES build) with no COMMAND that then gets dependencies assigned using add_dependencies(project+context-executes some-executes-item). That way only minimal changes to Maker.BuildDependencies() (and the extra step in superlists) are needed in cbuild2cmake. After changing this, triggering the all target after build seems to no longer be required.