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

Symlink 7z when USE_SYSTEM_P7ZIP #43005

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

haampie
Copy link
Contributor

@haampie haampie commented Nov 8, 2021

It seems like a symlink to 7z was missing from <julia prefix>/libexec/ upon install with system p7zip. Normally it's installed from build_bindir, so this PR creates a symlink to system 7z in that build dir.

In any case, it seems convenient to have a symlinked 7z like we symlink system libs, so that 7z doesn't have to be in the PATH when running julia.

I'm pretty sure this PR needs some improvement, I have no clue if $(shell which 7z...) is OK (in particular with $(EXE) :D).

I did not add code to follow symlinks like is done for shared libs (it seems resolve_path is intended for libraries)

@haampie haampie closed this Nov 9, 2021
@haampie haampie reopened this Nov 9, 2021
@ViralBShah ViralBShah added the building Build system, or building Julia or its dependencies label Nov 9, 2021
@ViralBShah
Copy link
Member

This one may need @staticfloat to look at it.

@staticfloat
Copy link
Member

What's the usecase for this? When you run USE_SYSTEM_XYZ=1, you're basically declaring that you don't want your Julia to be redistributable, so you shouldn't need to care about the environment changing. Are there tools that try to directly open libexec/7z and fail?

@haampie
Copy link
Contributor Author

haampie commented Nov 10, 2021

Hi Elliot, I do care about keeping a fixed environment, as I'm building Julia with Spack, meaning there is a very specific install of p7zip for Julia, in its own prefix dir, and this prefix is not in the PATH at runtime by default.

Of course I can symlink 7z after make install in the Spack recipe, but it looks to me the missing symlink for 7z was an oversight / exception, given that Julia symlinks all system libraries by default. If Julia fixes system libs by symlinking them so that they aren't influenced by LD_LIBRARY_PATH, then why not fix executables by symlinking them so that they aren't influenced by PATH.

Also for troubleshooting builds or development, it would not hurt to have the symlink already in the in-tree build

base/Makefile Outdated Show resolved Hide resolved
@staticfloat
Copy link
Member

Yeah, that totally makes sense. Thanks for giving the motivation so clearly!

Co-authored-by: Elliot Saba <staticfloat@gmail.com>
@haampie
Copy link
Contributor Author

haampie commented Nov 10, 2021

Thanks for the review! Should this be backported like #43000? It could be seen as a fix for a build issue

@staticfloat
Copy link
Member

@haampie have you tried this branch with your usecase to ensure that it works? If so, as long as CI is green, I think this is good to merge and to backport.

@haampie
Copy link
Contributor Author

haampie commented Nov 11, 2021

I'll verify it works for me tomorrow!

@haampie
Copy link
Contributor Author

haampie commented Nov 12, 2021

Works well for me!

@staticfloat staticfloat merged commit 5e2894b into JuliaLang:master Nov 12, 2021
@haampie haampie deleted the fix/symlink-for-external-7z branch November 12, 2021 20:27
KristofferC pushed a commit that referenced this pull request Nov 15, 2021
* Symlink 7z

* Simplify

Co-authored-by: Elliot Saba <staticfloat@gmail.com>

Co-authored-by: Elliot Saba <staticfloat@gmail.com>
(cherry picked from commit 5e2894b)
daviehh pushed a commit to daviehh/julia that referenced this pull request Nov 16, 2021
* Symlink 7z

* Simplify

Co-authored-by: Elliot Saba <staticfloat@gmail.com>

Co-authored-by: Elliot Saba <staticfloat@gmail.com>
(cherry picked from commit 5e2894b)
haampie added a commit to haampie/julia that referenced this pull request Nov 24, 2021
* Symlink 7z

* Simplify

Co-authored-by: Elliot Saba <staticfloat@gmail.com>

Co-authored-by: Elliot Saba <staticfloat@gmail.com>
@nalimilan
Copy link
Member

I had missed this change, but it seems problematic to me. Julia shouldn't install a file called 7z in /usr/libexec, as it's going to pollute that directory. If several programs do that they are going to conflict. If we really want this, I think it should rather be in a libexec/julia subdirectory, just like we do for libraries under lib/julia.

In practical terms, I have to manually remove this link to build the Fedora package to avoid this.

(BTW, I don't think it should have been backported at this stage as it's really a new feature, not a fix.)

@haampie
Copy link
Contributor Author

haampie commented Dec 6, 2021

I missed this comment. You mean you're running make install with prefix=/ and it clashes with /usr/libexec/7z? (who installs 7z there apart from julia?) My issue was that when prefix is anything other than /, p7zip_jll may not find the 7z executable. I requested a backport, since it looked like an oversight: we have symlinks for system libs, but not for libexec/7z)

@staticfloat
Copy link
Member

Placing it in libexec/julia would be a reasonable fix.

@nalimilan
Copy link
Member

You mean you're running make install with prefix=/ and it clashes with /usr/libexec/7z? (who installs 7z there apart from julia?)

My concern is more theoretical: as a packager, I'm not supposed to install to system directories files whose name may conflict with other apps. Given that 7z is a popular app, it's totally possible that another package would have the same needs as Julia, and if both install a file to /usr/libexec we will have conflicts.

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
* Symlink 7z

* Simplify

Co-authored-by: Elliot Saba <staticfloat@gmail.com>

Co-authored-by: Elliot Saba <staticfloat@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
* Symlink 7z

* Simplify

Co-authored-by: Elliot Saba <staticfloat@gmail.com>

Co-authored-by: Elliot Saba <staticfloat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants