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

move complex ldconfig code into dependency build time #277

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 12, 2017

this is in preparation for base Julia to drop this function

end
end
else
let fmt = Regex("^\\s:-l(\\S+)\\.\\S+\\s(.+)\$")
Copy link
Contributor

@iblislin iblislin Apr 16, 2017

Choose a reason for hiding this comment

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

Hi @vtjnash ,
FreeBSD has this pattern

fmt = r"^\s+\d+:-l(\S+)\.\S+ => (.+)$"

Copy link
Contributor

@iblislin iblislin Apr 16, 2017

Choose a reason for hiding this comment

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

Here are some samples:

└─[iblis@abeing]% ldconfig -r | head
/var/run/ld-elf.so.hints:
        search directories: /lib:/usr/lib:/usr/lib/compat:/usr/local/lib:/usr/local/lib/compat/pkg:/usr/local/lib/fcitx/qt:/usr/local/lib/gcc5:/usr/local/lib/gcc6:/usr/local/lib/graphviz:/usr/local/lib/mysql:/usr/local/lib/nss:/usr/local/lib/opencollada:/usr/local/lib/perl5/5.24/mach/CORE:/usr/local/lib/qt4:/usr/local/lib/samba4:/usr/local/lib/wine:/usr/local/llvm36/lib:/usr/local/llvm37/lib:/usr/local/llvm38/lib:/usr/local/llvm39/lib:/usr/local/mpi/openmpi/lib:/usr/local/mpi/openmpi/lib/openmpi:/usr/local/pypy-5.6/bin
        0:-lalias.7 => /lib/libalias.so.7
        1:-lncursesw.8 => /lib/libncursesw.so.8
        2:-lavl.2 => /lib/libavl.so.2
        3:-lbegemot.4 => /lib/libbegemot.so.4
        4:-lbsdxml.4 => /lib/libbsdxml.so.4
        5:-lc.7 => /lib/libc.so.7
        6:-lcam.6 => /lib/libcam.so.6
        7:-lcrypt.5 => /lib/libcrypt.so.5
julia> f = r"^\s+\d+:-l(\S+)\.\S+ => (.+)$"
r"^\s+\d+:-l(\S+)\.\S+ => (.+)$"

julia> line = "        0:-lalias.7 => /lib/libalias.so.7"
"        0:-lalias.7 => /lib/libalias.so.7"

julia> match(f, line)
RegexMatch("        0:-lalias.7 => /lib/libalias.so.7", 1="alias", 2="/lib/libalias.so.7")

Copy link
Member Author

Choose a reason for hiding this comment

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

How does it distinguish between something like libLLVM-3.8.so.1 (with digits in the name) and libperl.so.5.22 (with multiple digits in the version number)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just checked - FreeBSD states that it must have exactly one digit following the dot. Thanks for checking up on this! :)

@codecov-io
Copy link

codecov-io commented Apr 18, 2017

Codecov Report

Merging #277 into master will increase coverage by 36.17%.
The diff coverage is 51.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #277       +/-   ##
===========================================
+ Coverage    0.14%   36.32%   +36.17%     
===========================================
  Files           4        4               
  Lines         683      870      +187     
===========================================
+ Hits            1      316      +315     
+ Misses        682      554      -128
Impacted Files Coverage Δ
src/dependencies.jl 39.95% <51.61%> (+39.95%) ⬆️
src/BinDeps.jl 23.36% <0%> (+22.91%) ⬆️
src/show.jl 62.5% <0%> (+62.5%) ⬆️
src/debug.jl 86.04% <0%> (+86.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b7685a...1399706. Read the comment docs.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 25, 2017

bump.

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2017

How widely have you tested this?

@vtjnash
Copy link
Member Author

vtjnash commented Apr 27, 2017

Ubuntu and FreeBSD. It doesn't have any meaning on Mac OS or Windows. It's also much more extensible than the current code (which is hard coded in C in base, and sometimes fails in our CI tests - although we just hide the error), so it will be much easier to add support for new OS configurations we discover later.

@tkelman
Copy link
Contributor

tkelman commented Apr 27, 2017

I meant how many packages.

let ldconfig_arch = Dict(:i686 => "x32",
:x86_64 => "x86-64",
:aarch64 => "AArch64"),
arch = get(ldconfig_arch, Sys.ARCH, ""),
Copy link
Contributor

Choose a reason for hiding this comment

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

Sys.ARCH can end up as i586, possibly i386 or i486 on some distro builds

@vtjnash
Copy link
Member Author

vtjnash commented Apr 27, 2017

Ah, thanks. I thought we normalized all of those, but checking our Make.inc file, I see that we don't.

@musm
Copy link
Contributor

musm commented May 8, 2017

bump?

this is in preparation for base Julia to drop this function
@vtjnash vtjnash merged commit cd6e61a into master Jun 14, 2017
@vtjnash vtjnash deleted the jn/read-sonames branch June 14, 2017 04:28
@tkelman
Copy link
Contributor

tkelman commented Jun 14, 2017

Did you test this against any packages?

@tkelman
Copy link
Contributor

tkelman commented Jul 16, 2017

I repeat, how many packages did you test this against?

@tkelman
Copy link
Contributor

tkelman commented Jul 24, 2017

Can you give a specific example where the old code does the wrong thing?

@tkelman
Copy link
Contributor

tkelman commented Aug 8, 2017

This is causing problems downstream, see JuliaStats/Rmath.jl#38

@tkelman
Copy link
Contributor

tkelman commented Aug 8, 2017

Without an actual counterexample, I see zero advantage whatsoever to using this version of the code on versions of julia prior to JuliaLang/julia#22886

@vtjnash
Copy link
Member Author

vtjnash commented Aug 8, 2017

Sure. Can you fix the METADATA version bound to require julia 0.6.1-

@tkelman
Copy link
Contributor

tkelman commented Aug 8, 2017

That fix isn't in every 0.6.1- version and it's not guaranteed to be in 0.6.1 release until it's been demonstrated to not break anything.

You also haven't provided a concrete test case where the old code did the wrong thing, so I'm not convinced this version is ever preferable.

tkelman added a commit that referenced this pull request Oct 6, 2017
use the code path from before #277 on versions of julia where the new code freezes
ref JuliaStats/Rmath.jl#38
ararslan pushed a commit that referenced this pull request Nov 6, 2017
use the code path from before #277 on versions of julia where the new code freezes
ref JuliaStats/Rmath.jl#38
ararslan pushed a commit that referenced this pull request Nov 15, 2017
use the code path from before #277 on versions of julia where the new code freezes
ref JuliaStats/Rmath.jl#38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants