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

Use get_libdir() according to the API documentation #13479

Closed
wants to merge 2 commits into from

Conversation

XenHat
Copy link

@XenHat XenHat commented Oct 28, 2019

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. labels Oct 28, 2019
@XenHat XenHat force-pushed the master branch 2 times, most recently from 2545eee to 6bb386e Compare October 28, 2019 19:54
@XenHat
Copy link
Author

XenHat commented Oct 28, 2019

Sorry for the push spam, I'm not used to sign off my commits

Bug: https://bugs.gentoo.org/691454

Signed-off-by: David Turenne <xenhat.hex@gmail.com>
@XenHat XenHat changed the title Use get_libdir() according to the API documentation Use get_libdir() according to the API documentation [please reassign] Oct 28, 2019
@gentoo-bot gentoo-bot changed the title Use get_libdir() according to the API documentation [please reassign] Use get_libdir() according to the API documentation Oct 28, 2019
@gentoo-bot
Copy link

Pull Request assignment

Submitter: @XenHat
Areas affected: ebuilds
Packages affected: sys-boot/refind

sys-boot/refind: @sveyret, @gentoo/proxy-maint

Linked bugs

Bugs linked: 691454


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

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. and removed assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. labels Oct 28, 2019
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2019-10-28 21:18 UTC
Newest commit scanned: ccc5dd8
Status: ✅ good

No issues found

# Prepare UDK workspace
if ! use gnuefi; then
mkdir "${UDK_WORKSPACE}" || die
ln -s "${EPREFIX}/usr/lib/udk/"{Mde,IntelFramework}{,Module}Pkg \
ln -s "${EPREFIX}${LIB_DIR}/udk/"{Mde,IntelFramework}{,Module}Pkg \
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried compiling rEFInd without the gnuefi flag (this would require a change in Gentoo profile)? Because I'm not sure that UDK installation is already done in the correct library…

Copy link
Author

Choose a reason for hiding this comment

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

I can try it the next time I have some time to work on my laptop.

Copy link
Author

Choose a reason for hiding this comment

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

I will try this now

@sveyret
Copy link
Contributor

sveyret commented Oct 29, 2019

Thank you for the PR @XenHat.
Sorry, but I am not sure I understood your last comment on the issue. Do you mean that you managed to install rEFInd v0.11.4 without the need to patch it? This would not be so surprising, because a lot of get_libdir() has been added in the compilation process since v0.10.4.
Because I am only proxy maintainer, I will let a real dev from the proxy maint team (@mgorny?) review it.

@XenHat
Copy link
Author

XenHat commented Oct 31, 2019

Thank you for the PR @XenHat.
Sorry, but I am not sure I understood your last comment on the issue. Do you mean that you managed to install rEFInd v0.11.4 without the need to patch it? This would not be so surprising, because a lot of get_libdir() has been added in the compilation process since v0.10.4.
Because I am only proxy maintainer, I will let a real dev from the proxy maint team (@mgorny?) review it.

That is correct. I did these changes before trying to emerge v0.11.4, assuming it would also not emerge. Then my machine broke during a profile upgrade, and after fully re-installing it, it emerged without problems. If the patch is uneccessary, no harm done.

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

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

@sveyret so what do you think? You can approve or disapprove. Have you, as a maintainer, tested that it doesn't break revdeps? I don't see any harm in this, but I'm not familiar with refind. If it needs to be in /usr/lib or not.

@@ -21,6 +21,7 @@ DEPEND="gnuefi? ( >=sys-boot/gnu-efi-3.0.2 )
DOCS=(README.txt)
PATCHES=("${FILESDIR}/makefile.patch")
UDK_WORKSPACE="${T}/udk"
LIBDIR="/usr/lib"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, you define this variable, but don't use it anywhere. Does the upstream makefile need it? Why not make this into get_libdir correctly, that should work with 17.1 profiles?

Copy link
Author

Choose a reason for hiding this comment

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

That's a honest tired mistake.

@@ -49,11 +50,11 @@ src_prepare() {

# bug 598647 - PIE not supported
sed -e '/^CFLAGS/s:$: -fno-PIE:' -i Make.common || die

LIB_DIR="$(get_libdir)" || die
Copy link
Member

Choose a reason for hiding this comment

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

get_libdir is a built-in function and doesn't need to die.

Copy link
Author

Choose a reason for hiding this comment

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

That was more to fail if it could not get libdir

@sveyret
Copy link
Contributor

sveyret commented Dec 7, 2019

@juippis I did not have time to make any test with this patch (and probably won't have time to make any in the coming weeks). But the conclusion of our discussion with @XenHat was that:

  • some of the changes are suspicious (could break compilation without the gnuefi flag);
  • the changes are however useless because this release of refind was already modified to use get_libdir where needed.

So I would suggest to close this PR without a merge. @XenHat, do you agree?

@XenHat
Copy link
Author

XenHat commented Dec 7, 2019

I just upgraded my computer and I have more than enough processing power to do some tests. I could do them for you, or we can close this. Either works :)

@XenHat
Copy link
Author

XenHat commented Dec 7, 2019

@juippis @sveyret if you want to provide a list of things to test, I can automate and/or run them manually for you 👍

@XenHat
Copy link
Author

XenHat commented Dec 7, 2019

I could not get portage to compile refind without gnuefi, presumably because I use openRC, not systemd. But so far, it builds and refind-install as well as refind-mkdefault suceeds.

Whether or not those changes are required is still left to debate, but they don't seem to break on OpenRC.

@XenHat
Copy link
Author

XenHat commented Dec 7, 2019

Closing for now, re-open if this needs to be revisited.

@XenHat XenHat closed this Dec 7, 2019
XenHat added a commit to XenHat/xenhat-overlay that referenced this pull request Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
5 participants