search

JuliaLang / Compat.jl

Compatibility across Julia versions
Other
145 stars 117 forks source link

Add Base internal function LIBEXECDIR #676

Closed musm closed 4 years ago

musm commented 4 years ago

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

martinholters commented 4 years ago

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 commented 4 years ago

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

musm commented 4 years ago

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 commented 4 years ago

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

https://github.com/JuliaLang/julia/pull/33125 moved things into a new libexec folder (backported to rc-3 https://github.com/JuliaLang/julia/pull/33221)

https://github.com/JuliaLang/julia/pull/33777 (backported to rc-5 https://github.com/JuliaLang/julia/pull/33630)

cc @staticfloat

musm commented 4 years ago

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 commented 4 years ago

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

martinholters commented 4 years ago

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

https://github.com/JuliaLang/Compat.jl/blob/0e8446145bfdb84c655537f3a5643f160adf0a20/src/deprecated.jl#L5

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()__.

musm commented 4 years ago

Great thanks, the problem was with the Sys usage

musm commented 4 years ago

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.