-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
e48f28f
to
e1c94bd
Compare
Thanks for cleaning up the old files. I've requested a few minor changes. |
116ec1e
to
5f895c8
Compare
5f895c8
to
088482b
Compare
@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 |
@tresf my first guess would be to just pass that via I don't know the full picture so I might just be rambling nonsense here, though :D |
Perfect. |
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: |
@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" |
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. |
Agreed, this really makes it feel redundant.
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. |
c48a81d
to
e0915b8
Compare
@DomClark updated that, and did that for pure Linux build as well to match. Works like a charm. Anyway, I'd say a re-review is in order :) |
e0915b8
to
7567a80
Compare
(includes removal of ancient Ubuntu 14 stuff)