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

Commit d74fe33 breaks project post-build execute steps #244

Closed
EaselinkBachmann opened this issue Jun 18, 2024 · 4 comments
Closed

Commit d74fe33 breaks project post-build execute steps #244

EaselinkBachmann opened this issue Jun 18, 2024 · 4 comments
Assignees

Comments

@EaselinkBachmann
Copy link

EaselinkBachmann commented Jun 18, 2024

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

  • Running cbuild --cbuild2cmake mysolution.csolution.yml should build all projects and their pre- and post-build steps.
  • Running cbuild --cbuild2cmake mysolution.csolution.yml --context myproject+context should build that project and its pre- and post-build steps.

Actual Behaviour

  • Running cbuild --cbuild2cmake mysolution.csolution.yml builds all projects and their pre-build steps, but no post-build steps (this was working before d74fe33)
  • Running cbuild --cbuild2cmake mysolution.csolution.yml --context myproject+context builds that project, but no post-build steps (this never worked)

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
Copy link
Collaborator

@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
Copy link
Author

EaselinkBachmann commented Jun 18, 2024

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
Copy link
Collaborator

The regression was fixed in #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.

@jkrech jkrech closed this as completed Jul 2, 2024
@EaselinkBachmann
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

4 participants