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

Remove 32 bit CI support for windows builds #7619

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Rossmaxx
Copy link
Contributor

closes: #7286
Linux and Mac OS didn't have first class 32 bit support in the CI since we started. It's been 20 years since 32 bit support has been obsoleted and per surveys, there is no hardware in use anymore.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@FyiurAmron
Copy link
Contributor

FyiurAmron commented Dec 10, 2024

TBH, I'd go with just removing the 32-related value from the matrix arrays (I guess it's two places in total ATM), and doing nothing more here. Reasoning is threefold:

  1. it's more readable and self-explanatory to see a variable, much less so than a magic string interfix (even if a common one like 64), and it makes it obvious where the exact value is related to the target arch (changeable) and where it's related to the arch of the build runner itself (fixed),
  2. drastically reduces the size of the commit - less changes, less noise, less possible errors due to fat fingers etc.,
  3. in the (however unlikely) case someone wants to still execute a 32-bit build, in a personal fork e.g. (retrocomputing fans are still out there : ), it'll allow them to use the CI code basically verbatim.

This is not a particularly strong opinion on my side, since I am aware that all of the 3 above can be argued about to some extend, but still, that would be my opinion here.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Dec 10, 2024

I could have done the simpler way like you suggested but felt like I had to do a "clean up". I'll revert if someone else says about it.

@tresf
Copy link
Member

tresf commented Dec 10, 2024

TBH, I'd go with just removing the 32-related value from the matrix arrays (I guess it's two places in total ATM), and doing nothing more here. Reasoning is threefold:

I politely disagree, it's the CI pipeline and it should be represented as simple as possible.

in the (however unlikely) case someone wants to still execute a 32-bit build, in a personal fork e.g. (retrocomputing fans are still out there : ), it'll allow them to use the CI code basically verbatim.

This PR doesn't touch the viability of 32-bit builds, just our CI's instruction to perform them. For example, @Rossmaxx didn't remove the 32-bit cmake files, so what you're describing is still possible. By leaving the matrix in the CI is essentially still "supporting" it, which we want to take a strong stance against doing. IMO, the best argument to keep the matrix is if we were able to offer Windows ARM64 functionality (which I don't think we're ready for).

set(WIN64 FALSE)
set(CMAKE_SYSTEM_PROCESSOR i686)
include(${CMAKE_CURRENT_LIST_DIR}/common/MinGW-W64.cmake)

Copy link
Member

@tresf tresf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except the comment that you've already left a reminder to remove.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@FyiurAmron
Copy link
Contributor

FyiurAmron commented Dec 10, 2024

I politely disagree, it's the CI pipeline and it should be represented as simple as possible.

like I said, this is not a strong opinion from me, nor a hill I'll die on. As long as I won't have to worry about maintaining 32-bit Win compatibility anymore when hacking LMMS code, I'm fine, it's still a +1 from me :D

disclaimer: I write and maintain pipelines for a living, and been a SRE before that. I don't want LMMS to become a job for me, ever... do whatever you want with the pipelines, as long as they work :D

@Rossmaxx Rossmaxx requested a review from tresf December 11, 2024 10:11
@tresf
Copy link
Member

tresf commented Dec 11, 2024

I politely disagree, it's the CI pipeline and it should be represented as simple as possible.

like I said, this is not a strong opinion from me, nor a hill I'll die on. As long as I won't have to worry about maintaining 32-bit Win compatibility anymore when hacking LMMS code, I'm fine, it's still a +1 from me :D

disclaimer: I write and maintain pipelines for a living, and been a SRE before that. I don't want LMMS to become a job for me, ever... do whatever you want with the pipelines, as long as they work :D

Coincidentally, yesterday, I made a two element matrix into a one-element matrix and then back to a two-element matrix, so my own words immediately haunted me. 🤦 My opinion has been swayed a bit, most likely by karma. I too am fine either way. :)

@tresf tresf requested a review from DomClark December 11, 2024 18:28
@tresf
Copy link
Member

tresf commented Dec 11, 2024

Kindly asking @DomClark for review if available. If not, I'm fine merging this as-is.

@FyiurAmron
Copy link
Contributor

Coincidentally, yesterday, I made a two element matrix into a one-element matrix and then back to a two-element matrix, so my own words immediately haunted me. 🤦 My opinion has been swayed a bit, most likely by karma. I too am fine either way. :)

High five, karma can be a real bitch :D I believe that the experience and pragmaticism from hundreds of sleepless nights caused by trivialities like this in the end changes and teaches us more effectively than any article, rationale or discussion :)

@tresf
Copy link
Member

tresf commented Dec 13, 2024

Kindly asking @DomClark for review if available. If not, I'm fine merging this as-is.

Friendly bump before we merge. <3

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.

Drop 32 bit support from CI.
3 participants