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

minor cleanup of MinGW-related scripts #7327

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

FyiurAmron
Copy link
Contributor

(includes removal of ancient Ubuntu 14 stuff)

@FyiurAmron FyiurAmron force-pushed the clean-up-old-MinGW-scripts branch 3 times, most recently from e48f28f to e1c94bd Compare June 17, 2024 21:24
@FyiurAmron
Copy link
Contributor Author

CC @DomClark @messmerd @tresf , if any of you can review/merge this I would be really grateful. I'm aiming at streamlining and revamping the MinGW/MSYS builds as much as possible here, step by step. Any comments are welcome.

cmake/build_mingw.sh Outdated Show resolved Hide resolved
cmake/build_mingw.sh Outdated Show resolved Hide resolved
@tresf
Copy link
Member

tresf commented Jun 18, 2024

Thanks for cleaning up the old files. I've requested a few minor changes.

@FyiurAmron FyiurAmron force-pushed the clean-up-old-MinGW-scripts branch 2 times, most recently from 116ec1e to 5f895c8 Compare June 19, 2024 13:18
@FyiurAmron FyiurAmron requested a review from tresf June 19, 2024 13:42
@tresf
Copy link
Member

tresf commented Jun 19, 2024

@FyiurAmron thanks. One more thought, and this isn't required, but how do you feel about parameters? For example, once #7252 is merged, we'll be able to provide some debugging commands for makensis (and friends), but only via WANT_DEBUG_CPACK=true. In your experience, how would you ideally provide these to a mingw scripted build?

@FyiurAmron
Copy link
Contributor Author

FyiurAmron commented Jun 19, 2024

@tresf my first guess would be to just pass that via CMAKE_OPTS - the MinGW script already consumes that env var and it's ubiquitous in our CI/helpers, so I'd just append -DWANT_DEBUG_CPACK=ON to $CMAKE_OPTS one way or the other (e.g. CMAKE_OPTS="-DWANT_DEBUG_CPACK=ON $CMAKE_OPTS" or CI's env or whatever the exact use case would be`), that should work well I think.

I don't know the full picture so I might just be rambling nonsense here, though :D

@tresf
Copy link
Member

tresf commented Jun 19, 2024

@tresf my first guess would be to just pass that via CMAKE_OPTS

Perfect.

@DomClark
Copy link
Member

IMO the MinGW wrapper script would ideally forward its arguments to CMake, so it can be used as a drop-in replacement. For macOS and MSVC builds (and Linux in #7316), we currently invoke CMake with a command like the following: cmake -S . -B build -DFOO=BAR. The build_mingw.sh script currently hardcodes an older command signature where the source directory is passed as a plain argument, and the build directory is assumed to be the current directory. It would be nice to be able to write ./cmake/build_win32.sh -S . -B build -DFOO=BAR, as opposed to the current mkdir build && cd build && CMAKE_OPTS="-DFOO=BAR" ../cmake/build_win32.sh.

@messmerd
Copy link
Member

@DomClark Maybe we could perform the tasks of the build script in the CMake toolchain file instead, and then just use

cmake -S . -B build -DFOO=BAR --toolchain "./cmake/toolchains/Ubuntu-MinGW-W64-${{ matrix.arch }}.cmake"

@DomClark
Copy link
Member

That's a possibility too. IIRC in the past the arguments have been in favour of maintaining scripts to automatically determine the toolchain file, in order to make things "just work", at which point it doesn't really matter where those tasks go. But MSVC users need to specify the toolchain themselves anyway, and the average user isn't going to be cross-compiling with MinGW, so perhaps the scripts don't offer too much benefit.

@tresf
Copy link
Member

tresf commented Jun 20, 2024

But MSVC users need to specify the toolchain themselves anyway

Agreed, this really makes it feel redundant.

the average user isn't going to be cross-compiling with MinGW

I'm not sure what "the average user" is, but I do agree that it's trivial to just add this to our documentation. In the past, when regular users would compile the software themselves, it was because we lacked a nightly release. Now we have a nightly release, this is much less likely to be used... so I too believe this script to be overkill.

Note, this script has been around since '08, so it's been kept around mostly as a tradition only: d3516cd.

Back then -- prior to CMake -- the compiler flags were much more complex to provide at the terminal.

@FyiurAmron FyiurAmron force-pushed the clean-up-old-MinGW-scripts branch 4 times, most recently from c48a81d to e0915b8 Compare June 27, 2024 16:00
@FyiurAmron
Copy link
Contributor Author

FyiurAmron commented Jun 27, 2024

@DomClark updated that, and did that for pure Linux build as well to match. Works like a charm.
@messmerd spot-on.
@tresf I concur. I removed the shell scripts, and everything still works like a charm, with just the toolchain files. IMVHO this was an overkill like Dom said: MSYS2 works perfectly without toolchain files (I'm using #7283 as a proof-of-concept for that, it works locally without any additional config for me), and MinGW Linux-to-Windows cross-compilation outside of CI is really an edge case... and even then it's still a one-liner that can be copy-pasted from the CI script if needed.

Anyway, I'd say a re-review is in order :)

@tresf tresf merged commit 118ca4e into LMMS:master Jun 28, 2024
10 checks passed
@FyiurAmron FyiurAmron deleted the clean-up-old-MinGW-scripts branch June 28, 2024 18:44
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