lukka / run-cmake

GitHub Action to build C++ applications with CMake (CMakePresets.json), Ninja and vcpkg on GitHub.
MIT License
173 stars 19 forks source link

'configurePreset' option is wrongly required #81

Closed quyykk closed 1 year ago

quyykk commented 1 year ago

According to the action.yml file, 'configurePreset' is not a required input. However, if you don't provide it you get the following error:

Error: Input required and not supplied: configurePreset
    at Object.getInput (/home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:10092:15)
    at ActionLib.getInput (/home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:9160:26)
    at new CMakeRunner (/home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:4033:56)
    at Function.<anonymous> (/home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:4042:37)
    at Generator.next (<anonymous>)
    at /home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:4010:71
    at new Promise (<anonymous>)
    at module.exports.214.__awaiter (/home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:4006:12)
    at /home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:4041:111
    at Object.using (/home/runner/work/_actions/lukka/run-cmake/v10/dist/index.js:12975:18)

Since I'm using 'configurePresetCmdString', I simply put a random string as input to 'configurePreset' to make it work (and the action correctly uses the command string instead of the random configure preset). However, it would be nice if I didn't have to pass a random value to 'configurePreset'.

Same thing goes for 'buildPreset'.

Thanks!

lukka commented 1 year ago

Indeed it looks like the place where the configurePreset is mandatory is located at this line.

It should not happening for the buildPreset, but evidently this is happening as well.

quyykk commented 1 year ago

It should not happening for the buildPreset, but evidently this is happening as well.

Now that you mention it, I've never actually tested it with buildPreset since it would always fail at the configure step first :sweat: I just assumed that it would fail as well.

Indeed it looks like the place where the configurePreset is mandatory is located at this line.

In this case can configurePreset be made optional? I don't see a reason to include it when using the command string input.

lukka commented 1 year ago

@quyykk right, the documentation states that the configurePreset is optional, when it is not. The docs should be fixed. It makes sense to me that the input is required as the project generation is required as well, otherwise the build would not work as the project files were missing.

Regarding your suggestion to make the configurePreset input optional, it can certainly be done at the cost of reworking the code and validate the changes and updating the documentation. I think another approach would be to go with the current code flow, which uses the presence of the configurePreset input, or the buildPreset input, as a way to identify whether to run or not the CMake configuration, or the build. That is if you do not specify configurePreset input, the project generation will not happen and your configurePresetCmdString will not be used either.

Rather than using a dummy string as the input, I think this would be ok for your use case:

- uses: lukka/run-cmake@v10
      with:
        configurePreset: <the value of this expression is set in the CONFIGURE_PRESET_NAME env var>
        configurePresetCmdString: <use that env var in your custom arguments>

Note that due to this long standing issue https://github.com/actions/toolkit/issues/272, the configurePreset cannot be empty (rather than a dummy string), which would be a more sensible way to indicate to the action the intention to run the project generation, without actually providing a dummy and meaningless string as a value.

Let me know if all of this makes sense to you as well, thanks.

quyykk commented 1 year ago

Using the environment variable is perfect, thank you!

the configurePreset cannot be empty (rather than a dummy string

Yeah I did encounter that limitation.

lukka commented 1 year ago

@quyykk documentation is fixed and augmented with a better explanation. Let me know if all looks good, thanks!

rcoup commented 1 year ago

@lukka in my example, I'm running CTest a few times in a ci job with different presets. For example:

      - name: "test: unit tests"
        uses: lukka/run-cmake@v10
        with:
          configurePreset: ci
          testPreset: ci-unit-tests

      - name: "test: e2e tests"
        uses: lukka/run-cmake@v10
        with:
          configurePreset: ci
          testPreset: ci-e2e-tests

But because configurePreset is required & populated it goes through configuration of the project for each test step — just wasting time and making noise in CI output. The same thing happens with separate configure + build invocations of run-cmake (eg: using vcpkg the configure is a big operation, and makes sense for it to be a separate CI step)

If configurePreset can't be optional/empty/null, could we add skipConfigure: true or similar to simply invoke ctest / cmake --build as appropriate?

lukka commented 1 year ago

@rcoup perhaps the configure step could be optional indeed, and as of now it is not, but it could be easily fixed. Questions: