Skip to content

intltool 0.51.0 and patch to fix error in regular expression when using Perl 5.26.0#5287

Closed
reedts wants to merge 4 commits intoeasybuilders:developfrom
reedts:20171103111704_new_pr_intltool0510
Closed

intltool 0.51.0 and patch to fix error in regular expression when using Perl 5.26.0#5287
reedts wants to merge 4 commits intoeasybuilders:developfrom
reedts:20171103111704_new_pr_intltool0510

Conversation

@reedts
Copy link
Copy Markdown
Contributor

@reedts reedts commented Nov 3, 2017

depends on #5288

@verdurin
Copy link
Copy Markdown
Member

verdurin commented Nov 3, 2017

Test report by @verdurin
SUCCESS
Build succeeded for 1 out of 1 (1 easyconfigs in this PR)
dell.lan - Linux fedora 26, Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, Python 2.7.13
See https://gist.github.com/f8a2087b747d62d3cced163bd6b7850b for a full test report.

@@ -0,0 +1,43 @@
--- intltool-0.51.0.orig/intltool-update.in 2015-03-09 02:39:54.000000000 +0100
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@reedts Please include a description of why the patch is needed at the top of the patch. I know you've named it for compatibility, but a more specific description would be useful.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also: author or reference to upstream source of the patch.

We generally also include the software name/version in the filename.

If the Perl version is irrelevant (unclear to me), I'd avoid using 5.26.0 in the patch filename.

So, I'd suggest naming it intltool-0.51.0-fix-Perl-regex.patch...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems to be needed for all versions of Perl since 5.23.0
I will update this next week.

@easybuilders easybuilders deleted a comment from boegelbot Nov 4, 2017
# https://bugs.launchpad.net/intltool/+bug/1696658
# This patch ist from:
# https://github.com/Alexpux/MSYS2-packages/blob/master/intltool/perl-5.22-compatibility.patch
patches = ['intltool-perl-5.26.0-fix-regex.patch']
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@reedts Since the patch is in no way specific to Perl 5.26.0, I would avoid including a version number in the patch filename?
Do include the intltool version though, since the patch is specific to this version of intltool.

So, I'd suggest: intltool-%(version)s-Perl-fix-regex.patch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My bad. The problem seems to be specific to the combination of intltool-0.51.0 and Perl-5.26.0 but the patch file itself is not specific to this combination. I will fix that.

# intltool bug with Perl 5.26
# https://bugs.launchpad.net/intltool/+bug/1696658
# This patch ist from:
# https://github.com/Alexpux/MSYS2-packages/blob/master/intltool/perl-5.22-compatibility.patch
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no need to include these comments here, they're already in the patch file itself?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe just something like "fix regex error with perl 5.26.0" at this point, so one can understand instantly what the patch is for when reading the easyconfig?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If people want to figure out what the patch does exactly, they can look into the patch file.

The patch filename could give a good first hint.

@boegel
Copy link
Copy Markdown
Member

boegel commented Nov 11, 2017

Ah crap, I overlooked this one when working on #5328...
I basically ended up doing the exact same thing as @reedts.

My apologies for overlooking your effort @reedts, this is basically already included in develop now via #5328...

@reedts
Copy link
Copy Markdown
Contributor Author

reedts commented Nov 17, 2017

@boegel All right then, so we can close this?

@boegel
Copy link
Copy Markdown
Member

boegel commented Nov 17, 2017

@reedts Yes.

@boegel boegel closed this Nov 17, 2017
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.

3 participants