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.

Sign up for GitHub

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

Jump to bottom

Patch Perl for macOS 11.x #57580

Merged
merged 5 commits into from
Aug 7, 2020
Merged

Patch Perl for macOS 11.x #57580

merged 5 commits into from
Aug 7, 2020

Conversation

BytesGuy
Copy link
Contributor

@BytesGuy BytesGuy commented Jul 6, 2020

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Perl will currently fail to build (Intel and Apple Silicon) as it is expecting to match the product version 10.x, whereas Big Sur has been bumped to 11.0. This is a temporary patch until support is available upstream (PR created: Perl/perl5#17946)

@BytesGuy BytesGuy mentioned this pull request Jul 6, 2020
Closed
9 tasks
@Bo98
Copy link
Member

Bo98 commented Jul 6, 2020

Makes sense. Let's give upstream a couple days to give comment first (PR diff URLs aren't stable as they change if any changes to the PR is made).

@BytesGuy
Copy link
Contributor Author

BytesGuy commented Jul 6, 2020

Makes sense. Let's give upstream a couple days to give comment first (PR diff URLs aren't stable as they change if any changes to the PR is made).

Ok, good call 🙂 Will follow up on this when my PR is reviewed! Meanwhile this might help anyone who is blocked.

I wonder how many of these kinds of issues we will see since being bumped to 11.0!

@claui claui added the 11 Big Sur is specifically affected label Jul 7, 2020
@ghost
Copy link

ghost commented Jul 21, 2020

https://eclecticlight.co/2020/07/21/big-sur-is-both-10-16-and-11-0-its-official/

@Bo98
Copy link
Member

Bo98 commented Jul 22, 2020

We won’t be enabling SYSTEM_VERSION_COMPAT in Homebrew, nor do we allow using an older SDK.

SYSTEM_VERSION_COMPAT could be applied to certain troublesome formulae where a patch to support 11.x is non-trivial, but this is a short term workaround and not a long term fix. Early indications show that the 11.x problem is not really as common as feared.

@BytesGuy
Copy link
Contributor Author

I think this is the only instance of an issue with the 11.0 version number so far. I’ll be following up on the PR to the Perl repo later, been offline for a few days so just catching up with things.

jonchang
jonchang suggested changes Jul 22, 2020
View reviewed changes
Formula/perl.rb Outdated
Comment on lines 22 to 23
url "https://github.com/Perl/perl5/pull/17946.diff?full_index=1"
sha256 "677a6e63412a4cfd2dc1fe16ccca53d789be5ac1acfb5ebb3130c40b51e5e94a"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please either use a commit URL or submit this to formula-patches. Note that the sha256 hash is already mismatched as you've pushed additional commits to that branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonchang Unless it's urgent we can wait until the upstream PR is merged in and use that as the patch, otherwise it just means updating the formula again. I'm pushing changes to the upstream PR shortly so hopefully it will be good enough to get merged in to resolve the issue for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed changes so let's hold for a few days to see what happens. If this doesn't get merged soon I'll just create a formula-patch as it will be easier to manage 👍

@BytesGuy BytesGuy mentioned this pull request Jul 22, 2020
Merged
@BytesGuy
Copy link
Contributor Author

@claui @jonchang I’ve opened Homebrew/formula-patches#293 so we can resolve this without waiting for the PR to be merged into Perl, it is taking some time!

@claui
Copy link
Contributor

claui commented Jul 26, 2020

I’ve opened Homebrew/formula-patches#293 so we can resolve this without waiting for the PR to be merged into Perl, it is taking some time!

Thanks! Merged the formula patch.

@BytesGuy
Copy link
Contributor Author

I’ve opened Homebrew/formula-patches#293 so we can resolve this without waiting for the PR to be merged into Perl, it is taking some time!

Thanks! Merged the formula patch.

Thanks 🙂 Just updated the formula to point to that patch, should be all good now!

claui
claui reviewed Jul 26, 2020
View reviewed changes
Formula/perl.rb Outdated Show resolved Hide resolved
claui added 2 commits July 27, 2020 09:59
@claui
perl: change comment to include the word remove
fa89ecf 
It’s helpful to have the word `remove` in the comment for temporary
patches because:

1. it’s a call to action (which helps with upkeep); and

2. whenever we want to prune outdated temporary patches, we have
something to grep for.
@claui
perl: fix Git revision of formula patch
b38cdf7
@SeekingMeaning SeekingMeaning added ready to merge PR can be merged once CI is green almost there PR is nearly ready to merge and removed ready to merge PR can be merged once CI is green labels Jul 30, 2020
@tale
Copy link

tale commented Aug 5, 2020

I just wanted to ask.
Any update on the merge status of this PR yet? It seems complete and has been for almost a week now.

@SeekingMeaning SeekingMeaning merged commit d18c0f2 into Homebrew:master Aug 7, 2020
@SeekingMeaning SeekingMeaning removed the almost there PR is nearly ready to merge label Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claui claui claui left review comments

@jonchang jonchang jonchang requested changes

Assignees
No one assigned
Labels
11 Big Sur is specifically affected
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BytesGuy @Bo98 @claui @tale @jonchang @SeekingMeaning