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 support for 64-bit SPARC as a separate architecture #9445

Merged
merged 1 commit into from Nov 18, 2023

Conversation

glaubitz
Copy link
Collaborator

@glaubitz glaubitz commented Nov 14, 2023

Previously, sparc64 was defined as an alias for the 32-bit SPARC architecture which was true while SPARC mainland was mostly 32 bits. More recently, 64-bit SPARC has become a port of its own, so it needs to be treated as a separate architecture.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 14, 2023

@glaubitz: thank you for the PR. CI indicates package hashes change and I suspect that also implies that this change should be guarded behind cabal-version 3.12. @phadej: am I right? BTW, is there any ready resource for developers explaining how to guard behind a cabal version or even when exactly this is necessary (just when inducing any PvP change in Cabal-syntax?)?

@glaubitz
Copy link
Collaborator Author

@glaubitz: thank you for the PR. CI indicates package hashes change and I suspect that also implies that this change should be guarded behind cabal-version 3.12. @phadej: am I right? BTW, is there any ready resource for developers explaining how to guard behind a cabal version or even when exactly this is necessary (just when inducing any PvP change in Cabal-syntax?)?

Is there a manual which explains how to update the hashes?

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 14, 2023

Just do it manually, @glaubitz. Check what CI expected, slap it in, commit.

@glaubitz glaubitz force-pushed the sparc64-support branch 2 times, most recently from 2f971f5 to 5196fed Compare November 14, 2023 14:00
@glaubitz
Copy link
Collaborator Author

Just do it manually, @glaubitz. Check what CI expected, slap it in, commit.

Should be done now and hopefully good for merging. ;)

Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

Good AFAICS

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Otherwise looks okay to me.

@@ -251,7 +252,6 @@ archAliases Strict _ = []
archAliases Compat _ = []
archAliases _ PPC = ["powerpc"]
archAliases _ PPC64 = ["powerpc64", "powerpc64le"]
archAliases _ Sparc = ["sparc64", "sun4"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the "sun4" alias here still needed for the 32-bit arch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have not seen it being used anywhere. And even Solaris is defaulting to a 64-bit userland these days.

@andreabedini
Copy link
Collaborator

@Mikolaj @ffaf1 @geekosaur I had a look and I am tempted to say that this does not require a bump in cabal-version.

The role of cabal-version is to help with forward compatibility (see #4899), i.e. let old cabal versions distinguish between something wrong and something from a future they can't understand.

Now, w.r.t to this change there are two factors:

  1. As far as I can tell, this change maps "sparc64" to a separate architecture rather than being an alias for "sparc". This alone does not change the allowed input syntax and both old and new versions of Cabal will still be able to parse the same file.
  2. The PR also removes the alias "sun4" and this could be a problem. But it is not, because Cabal is perfectly fine with architectures it does not know:
ghci> simpleParsec "x86_64" :: Maybe Arch
Just X86_64
ghci> simpleParsec "my-coffe-machine" :: Maybe Arch
Just (OtherArch "my-coffe-machine")

so also in this case the accepted syntax is not changed.

So 👍 from me.

Note: The hash changes because the data type has changed, but the API is not the same thing has the accepted syntax.

Note: Adding a constructor to an exported data type is a major version change.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 15, 2023

That's a relief. @glaubitz, feel free to set a merge label, as described in CONTRIBUTING. We'd still have 2 days for someone to correct us. Thank you, everybody!

@glaubitz glaubitz added the merge me Tell Mergify Bot to merge label Nov 16, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 18, 2023
Previously, sparc64 was defined as an alias for the 32-bit SPARC
architecture which was true while SPARC mainland was mostly 32
bits. More recently, 64-bit SPARC has become a port of its own,
so it needs to be treated as a separate architecture.
@glaubitz
Copy link
Collaborator Author

It says that there 3 checks that are failing and that seems to block the auto-merge.

Anyone can point me to what's failing?

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 18, 2023

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 18, 2023

rebase

✅ Nothing to do for rebase action

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Nov 18, 2023

@glaubitz thanks for the ping! The macos failure is transient and requires restarting, which I did. The rest is related to the merge bot Mergify and will have to be dealt with once the macos job succeeds.

@mergify mergify bot merged commit 88e4b00 into haskell:master Nov 18, 2023
47 checks passed
@ulysses4ever
Copy link
Collaborator

Hooray! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants