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

dev-libs/libxslt: version bump #7268

Closed
wants to merge 1 commit into from
Closed

dev-libs/libxslt: version bump #7268

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 23, 2018

Closes: https://bugs.gentoo.org/632214
Package-Manager: Portage-2.3.19, Repoman-2.3.6

@stha09
dev-libs/libxslt: version bump
6e418d9 
Closes: https://bugs.gentoo.org/632214
Package-Manager: Portage-2.3.19, Repoman-2.3.6
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: dev-libs/libxslt

dev-libs/libxslt: @gentoo/gnome

Bugs linked: 632214

In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

@gentoo-repo-qa-bot gentoo-repo-qa-bot added the assigned PR successfully assigned to the package maintainer(s). label Feb 23, 2018
@ghost
Copy link
Author

ghost commented Feb 23, 2018

I have only tested the ebuild on FreeBSD so far, because the glibc-2.26 patch was wrong there too.

@gentoo-repo-qa-bot gentoo-repo-qa-bot added the bug linked Bug/Closes found in footer, and cross-linked with the PR. label Feb 23, 2018
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2018-02-23 18:56 UTC
Newest commit scanned: 6e418d9
Status: ✅ good

No issues found

monsieurp
monsieurp suggested changes Feb 26, 2018
View reviewed changes
dev-libs/libxslt/libxslt-1.1.32.ebuild
src_prepare() {
default

DOCS=( AUTHORS ChangeLog FEATURES NEWS README TODO )
Copy link
Member

Choose a reason for hiding this comment

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

Please move this array outside the src_prepare() function.

dev-libs/libxslt/libxslt-1.1.32.ebuild
DOCS=( AUTHORS ChangeLog FEATURES NEWS README TODO )

# https://bugzilla.gnome.org/show_bug.cgi?id=684621
eapply "${FILESDIR}"/${PN}.m4-${PN}-1.1.26.patch
Copy link
Member

Choose a reason for hiding this comment

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

Put all patches into the PATCHES array outside this function as well.

dev-libs/libxslt/libxslt-1.1.32.ebuild
einstalldocs

if ! use examples; then
rm -rf "${ED}"/usr/share/doc/${PF}/examples
Copy link
Member

Choose a reason for hiding this comment

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

rm -r ... || die

dev-libs/libxslt/libxslt-1.1.32.ebuild

if ! use examples; then
rm -rf "${ED}"/usr/share/doc/${PF}/examples
rm -rf "${ED}"/usr/share/doc/${PF}/python/examples
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@leio
Copy link
Member

leio commented Feb 26, 2018 •
edited

Please don't make the requested changes for a version bump not proxy-maintained. Maybe in a separate commit though. That said:

  • rm -rf can really never die as probably ran with root permissions there, and || die is kind of redundant. I'm fine with adding it though. Prefix and stuff. So yeah, lets have it, useful and correct there actually on second thought.
  • DOCS in global scope or elsewhere is a maintainer style preference and isn't a place to change as non-maintainer. That said, I don't see a reason to not change it - things other than libxslt and libxml2 that we maintain have it in global scope. There is no practical difference - both cases end up in saved environment.
  • PATCHES array does not support conditional patching, that the prefix/darwin people have added, and at that point it is bad to use PATCHES array in global scope, as the order in which the patches there and the conditional one in src_prepare are applied isn't immediately obvious.

IOW, please disregard the review. I was planning to look over it as libxslt co-maintainer within a couple days now and merge it if all is fine after functional review. Some of these suggested changes would be wrong and would make me reject the PR. More useful would be knowing more handily if any actual changes beyond KEYWORDS dropping to ~arch was needed or not, though.. but will see after pulling the patch and diffing with previous version ebuild.

@monsieurp
Copy link
Member

rm -rf in an ebuild is plain wrong. But sure, disregard the review.

@leio
Copy link
Member

leio commented Feb 27, 2018 •
edited

oh, I missed the removal of -f, yeah, that's a good change too, sorry.

@ghost
Copy link
Author

ghost commented Mar 2, 2018

So just remove -f from all rm's?

@EvaSDK
Copy link
Contributor

EvaSDK commented Mar 5, 2018

Also, please make sure testsuite runs fine. I'm not sure this was the case last time I tried to bump this package and it is pretty critical.

@EvaSDK EvaSDK assigned leio and EvaSDK Mar 5, 2018
@charIes17
Copy link
Contributor

Please add closing bug 637310 to the commit message.
Closes: https://bugs.gentoo.org/637310

@leio
Copy link
Member

leio commented Apr 21, 2018 •
edited

I forgot about this PR before doing the bump from scratch; though unfortunately I found it afterwards from bugzilla comments, making me waste half an hour on the rm -r stuff, which obviously wasn't as easy as the suggested change, for zero actual benefits. Sorry for forgetting to work on top of this commit and had your work be wasted

@leio leio closed this Apr 21, 2018
@ghost ghost deleted the libxslt branch April 23, 2018 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@leio leio

@EvaSDK EvaSDK

Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR.
Projects
None yet
Milestone
No milestone
6 participants
@gentoo-repo-qa-bot @leio @monsieurp @EvaSDK @charIes17 @stha09