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

Add an example for how to use the github action with CMake #149

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

mstorsjo
Copy link
Contributor

This shows off a couple aspects; how to install the tool packages
with the right prefix (as you need the mingw version of CMake, not
the msys version), how to set a non-default project generator,
and mentions another gotcha from the generic GitHub Actions CMake
example/template.

@lazka
Copy link
Member

lazka commented Jul 16, 2021

Somewhat related, there was talk about changing the default cmake generator to Ninja in our build. Do you think that's a good idea?

@mstorsjo
Copy link
Contributor Author

Somewhat related, there was talk about changing the default cmake generator to Ninja in our build. Do you think that's a good idea?

I guess it could be, because the default one, which ends up looking for MSVC even outside of $PATH is kinda surprising.

I wonder if "MSYS Makefiles" would be the least surprising default, for unix projects that would do just cmake; make though?

@Biswa96
Copy link
Member

Biswa96 commented Jul 16, 2021

Is it correct to reflect ucrt and clang build matrix in docs? Those are somewhat considered as testing area now.

there was talk about changing the default cmake generator to Ninja in our build

I forgot the package name but if I change MSYS Makefiles to Ninja then CMake can not find wxWidgets as dependency.

@lazka
Copy link
Member

lazka commented Jul 16, 2021

I wonder if "MSYS Makefiles" would be the least surprising default, for unix projects that would do just cmake; make though?

yeah, likely less surprising, even if slower... thanks

Ideally we want people to use ninja though (to get rid of the cygwin dependency) and changing it from make to ninja after the fact will likely result in some breakage.

@mstorsjo
Copy link
Contributor Author

Is it correct to reflect ucrt and clang build matrix in docs? Those are somewhat considered as testing area now.

I would definitely advertise them here. This is for the case when you want to test building your own project and just want to have the bare toolchain available from msys2. If one builds a project and adds commands for installing specific packages that don't exist, then that'll probably be pretty clear to the user what happened. (But a link explaining what the different msystem options are would be good - I'll add one.)

I wonder if "MSYS Makefiles" would be the least surprising default, for unix projects that would do just cmake; make though?

yeah, likely less surprising, even if slower... thanks

Ideally we want people to use ninja though (to get rid of the cygwin dependency) and changing it from make to ninja after the fact will likely result in some breakage.

Yeah, for actual practical compilation, ninja clearly is the best. Maybe it's better to push for people to use that and make it default - it's not worse than before anyway (either you manually set the generator, and/or build using cmake --build to asbstract it away).

This shows off a couple aspects; how to install the tool packages
with the right prefix (as you need the mingw version of CMake, not
the msys version), how to set a non-default project generator,
and mentions another gotcha from the generic GitHub Actions CMake
example/template.
@lazka lazka merged commit 9f5f4ea into msys2:master Jul 16, 2021
@mstorsjo mstorsjo deleted the cmake-example branch July 16, 2021 10:30
@eine
Copy link
Collaborator

eine commented Jul 16, 2021

Please, let this kind of disruptive PRs alive for at least 24-48h before merging, so that maintainers/contributors can provide feedback, should they want and have time to.

In this case, I do not agree with adding this content to the main readme. We don't want to overload the readme with specific workflow examples for particular use cases. I do acknowledge that the information contributed by this PR is relevant and it should be available in the docs of MSYS2 as a whole, but it does not fit where it was written.

Precisely:

  • The usage of a prefix for including ucrt-x86_64 and clang-x86_64 is pertinent, because that's different from the x86_64 and i686 used before. However, there is no reason for repeating mingw-w64 in all the prefixes. Moreover, it's not specific to the cmake workflow. Therefore, I would suggest adding the following to https://github.com/msys2/setup-msys2#build-matrix:
    strategy:
      matrix:
        include:
          - { sys: mingw64, env: x86_64 }
          - { sys: mingw32, env: i686 }
          - { sys: ucrt64,  env: ucrt-x86_64 }  # Experimental!
          - { sys: clang64, env: clang-x86_64 } # Experimental!
    steps:
      - uses: msys2/setup-msys2@v2
        with:
          msystem: ${{matrix.sys}}
          install: mingw-w64-${{matrix.env}}-toolchain
  • The guidelines for using cmake are a very important content that should have a privileged location in MSYS2's docs. Using Ninja or MSYS2 Makefiles, invoking make or cmake --build, setting -DCMAKE_BUILD_TYPE, settting MSYS2_ARG_CONV_EXCL="-DCMAKE_INSTALL_PREFIX=" (not covered in this PR), etc. all of those are useful for any contributor/maintainer, and it's unrelated to setup-msys2.

  • Providing a ready-to-use workflow which users can copy and paste is out of the scope of this repo. If we wanted to provide that, which might be sensible, I would propose to take https://hdl.github.io/MINGW-packages/#_contributing as a reference. See https://hdl.github.io/MINGW-packages/#_testing_pkgbuild_recipes_downstream and https://github.com/hdl/MINGW-packages/blob/main/testing-workflow.yml. In the cmake example of this PR, https://github.com/hdl/MINGW-packages/blob/main/testing-workflow.yml#L24-L25 is missing. So, we should create an examples subdir, and put *.yml workflows there. Or, better, actually use those in CI (.github/workflows/example_*.yml) so that we ensure they don't get out of sync.

  • The first point of the comparison with "the GitHub Actions CMake examples/templates" might be pertinent, but I find it to be incomplete. It's possible to use cygwin -u to work around the backslashes. Hence, avoiding github.workspace is one option only, not necessarily the most desirable.

    • The second point is general knowledge about using CMake on MSYS2, not specific to setup-msys2 or to GitHub Actions.

I don't feel like reverting this PR, because it is not a critical issue. However, I would be glad if a follow-up PR addressed those concerns.

EDIT

FTR, https://github.com/msys2/setup-msys2/commits/egs

@lazka
Copy link
Member

lazka commented Jul 16, 2021

Please, let this kind of disruptive PRs alive for at least 24-48h before merging, so that maintainers/contributors can provide feedback, should they want and have time to.

ok, sorry, I'll keep that in mind.

@mstorsjo
Copy link
Contributor Author

FTR, https://github.com/msys2/setup-msys2/commits/egs

Thanks for picking up the ball regarding moving the info to the right locations! I’m a little unavailable at the moment, but I’ll try to help out with what’s left to do when I have a bit more time again.

@eine
Copy link
Collaborator

eine commented Jul 19, 2021

@mstorsjo no rush! I created msys2/msys2.github.io#144 for tracking that enhancement.

@lazka
Copy link
Member

lazka commented Jul 19, 2021

I've created msys2/MINGW-packages#9176 for changing the default to ninja

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.

4 participants