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

Add Base internal function LIBEXECDIR #676

Closed
wants to merge 10 commits into from
Closed

Conversation

musm
Copy link
Contributor

@musm musm commented Nov 14, 2019

This is being backported to rc5, so I'm not sure if VERSION < v"1.4.0-DEV.445" is appropriate?

@martinholters
Copy link
Member

This is being backported to rc5, so I'm not sure if VERSION < v"1.4.0-DEV.445" is appropriate?

Yeah, that's tricky. We obviously don't want to mess with Base.LIBEXECDIR in 1.3. So maybe VERSION < v"1.3.0-rc5 || v"1.4" <= VERSION < v"1.4.0-DEV.445? Also, I guess we should wait with merging this until rc5 is out to be sure it really works there.

@martinholters martinholters added needs-news Needs an entry in the README needs-tests labels Nov 15, 2019
src/Compat.jl Outdated Show resolved Hide resolved
@martinholters
Copy link
Member

Is there a good way to test this? Make sure 7z.exe can be found on windows?

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

Unfortunately it's really hard to test this since it's an optional build dependency whether 7z.exe exists in that folder. As a result this is actually not even tested in Julia due to these difficulties.

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

I think I fixed up all the version logic here which is a bit complex due to all the backports

JuliaLang/julia#33125
moved things into a new libexec folder (backported to rc-3 JuliaLang/julia#33221)

JuliaLang/julia#33777 (backported to rc-5 JuliaLang/julia#33630)

cc @staticfloat

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

The assumption is that all commits before JuliaLang/julia#33125 don't have a libexec folder so we emulate it's behavior where the relevant external exe's are located simply in the root julia bin folder.

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

Not sure why this is failing on 1.3, since it works locally

src/Compat.jl Outdated
@@ -93,6 +93,19 @@ if VERSION < v"1.4.0-DEV.329"
Base.:∘(f, g, h...) = ∘(f ∘ g, h...)
end

# https://github.com/JuliaLang/julia/pull/33777
if v"1.4.0-DEV.172" <= VERSION < v"1.4.0-DEV.445"
libexecdir = Sys.iswindows() ? "..\\libexec" : "../libexec"
Copy link
Member

@martinholters martinholters Nov 16, 2019

Choose a reason for hiding this comment

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

Suggested change
libexecdir = Sys.iswindows() ? "..\\libexec" : "../libexec"
libexecdir = joinpath("..", "libexec")

(same below)

@martinholters
Copy link
Member

The test failure is due to resolving the Sys binding which makes this deprecation fail:

Base.@deprecate_binding Sys Base.Sys false

Probably explicitly using Base.Sys would work, or circumvent accessing Sys as I've suggested.

However, there is also this:

WARNING: eval into closed module Base:
Expr(:const, :LIBEXECDIR = "../libexec")
  ** incremental compilation may be fatally broken for this module **

You probably have to move the code into __init()__.

@martinholters martinholters removed the needs-news Needs an entry in the README label Nov 16, 2019
@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

Great thanks, the problem was with the Sys usage

@musm
Copy link
Contributor Author

musm commented Nov 16, 2019

I'm having second thoughts on whether we should include this. It's not apart of the stable API. Including it makes updating some packages easier, but I wonder if it's better to let them just handle it instead of including it as part of Compat.

# https://github.com/JuliaLang/julia/pull/33777
if v"1.4.0-DEV.172" <= VERSION < v"1.4.0-DEV.445"
@eval(Base, const LIBEXECDIR = $(joinpath("..", "libexec")))
elseif v"1.4" <= VERSION < v"1.4.0-DEV.172"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need also find the commit where this was moved, so this is not 100% correct in all cases.

@musm musm closed this Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants