Ignore error of symlink by junjihashimoto · Pull Request #83605 · NixOS/nixpkgs

junjihashimoto · 2020-03-28T16:03:20Z

Motivation for this change

When setupCompilerEnvironmentPhase, symlink fails for existing link.
I want to ignore the error.
This pr changes ln -s to ln -sf.

OS : macos

{
  "url": "https://github.com/nixos/nixpkgs-channels.git",
  "rev": "3320a06049fc259e87a2bd98f4cd42f15f746b96",
  "date": "2020-03-26T20:57:21-04:00",
  "sha256": "1g5l186d5xh187vdcpfsz1ff8s749949c1pclvzfkylpar09ldkl",
  "fetchSubmodules": false
}

https://github.com/hasktorch/hasktorch/runs/541719980

Things done

cdepillabout · 2020-03-29T01:02:25Z

This code was last changed by @infinisil in 8d4509e.

@infinisil Is it possible that there was a bug introduced in 8d4509e for when there are multiple *.dylib files that have the same name?

Is the fix that @junjihashimoto is proposing here correct?

nh2 · 2020-03-29T14:54:20Z

If -f is added we definitely need a (commented) explanation of what happens when multiple sources provide the same file (the last one will win), and why that makes sense.

Is it correct that there should be such conflicts in the first place?

junjihashimoto · 2020-03-29T15:27:52Z

@nh2
there are conf-files for each library(haskell's package) in $packageConfDir.
And dynamic-library-dirs is path of shared libraries the library uses.

For example,
Assuming we have three libraries (A, B, C),
we will have A.conf, B.conf and C.conf in $packageConfDir.
When A depends on B, and B depends on C, and C has libC.so,
we will take duplicate paths of libC.so.
But the duplicated paths are all the same, so I think we can ignore this error.

    for d in $(grep '^dynamic-library-dirs:' "$packageConfDir"/* | cut -d' ' -f2- | tr ' ' '\n' | sort -u); do
      for lib in "$d/"*.{dylib,so}; do
        ln -s "$lib" "$dynamicLinksDir"
        ln -sf "$lib" "$dynamicLinksDir"
      done
    done

gnprice · 2020-04-02T07:47:24Z

Is it possible that there was a bug introduced in 8d4509e for when there are multiple *.dylib files that have the same name?

I think, from reading that commit, that the code should try to create the same set of links before and after the commit. (Outside of cases of the bug that the commit was fixing.) If so, then this has probably behaved the same way since this lib/links/ directory was introduced in #25537.

When A depends on B, and B depends on C, and C has libC.so,
we will take duplicate paths of libC.so.
But the duplicated paths are all the same, so I think we can ignore this error.

If that's what's going on, then perhaps we can have the script check that: if the link already exists, it should point to the same path we were going to create it with. Then if it doesn't, we create it just like we do now.

junjihashimoto · 2020-04-03T07:39:49Z

pkgs/development/haskell-modules/generic-builder.nix

+    listDynamicLinks () {
+      for d in $(grep '^dynamic-library-dirs:' "$packageConfDir"/* | cut -d' ' -f2- | tr ' ' '\n' | sort -u); do
+        for lib in "$d/"*.{dylib,so} ; do
+          if [ -f "$lib" ] ; then


When we use linux, lib includes not only actual filenames but also *.dylib.
This [ -f "$lib" ] is used to delete *.dylib.

Mmm, I see -- because there are no files matching somedir/*.dylib?

Hmm, but this bit of code has always been this way -- in fact it's only since February in 019e86f that it mentions .so at all, and before that it was only .dylib:

for d in $(grep '^dynamic-library-dirs:' "$packageConfDir"/* | cut -d' ' -f2- | tr ' ' '\n' | sort -u); do - for lib in "$d/"*.dylib; do + for lib in "$d/"*.{dylib,so}; do ln -s "$lib" "$dynamicLinksDir"

This change should be a separate commit, in any event. Probably deserves some separate debugging and discussion, too. It sounds like an independent issue from the collision issue.

junjihashimoto · 2020-04-03T07:43:06Z

@gnprice
I update this pr to to generating duplicate links.
Is it fine like this?

cdepillabout · 2020-04-03T08:12:54Z

pkgs/development/haskell-modules/generic-builder.nix

-    for d in $(grep '^dynamic-library-dirs:' "$packageConfDir"/* | cut -d' ' -f2- | tr ' ' '\n' | sort -u); do
-      for lib in "$d/"*.{dylib,so}; do
-        ln -s "$lib" "$dynamicLinksDir"
+    listDynamicLinks () {


Could you add a comment on this function explaining what it is doing (and why it is needed)?

cdepillabout

I left one comment above.

Also, please rebase this on the haskell-updates branch.

gnprice

Thanks @junjihashimoto for the revisions! Comments below.

gnprice · 2020-04-03T17:05:14Z

pkgs/development/haskell-modules/generic-builder.nix

+    listDynamicLinks () {
+      for d in $(grep '^dynamic-library-dirs:' "$packageConfDir"/* | cut -d' ' -f2- | tr ' ' '\n' | sort -u); do
+        for lib in "$d/"*.{dylib,so} ; do
+          if [ -f "$lib" ] ; then


Mmm, I see -- because there are no files matching somedir/*.dylib?

Hmm, but this bit of code has always been this way -- in fact it's only since February in 019e86f that it mentions .so at all, and before that it was only .dylib:

for d in $(grep '^dynamic-library-dirs:' "$packageConfDir"/* | cut -d' ' -f2- | tr ' ' '\n' | sort -u); do - for lib in "$d/"*.dylib; do + for lib in "$d/"*.{dylib,so}; do ln -s "$lib" "$dynamicLinksDir"

This change should be a separate commit, in any event. Probably deserves some separate debugging and discussion, too. It sounds like an independent issue from the collision issue.

gnprice · 2020-04-03T17:15:47Z

pkgs/development/haskell-modules/generic-builder.nix

+        for lib in "$d/"*.{dylib,so} ; do
+          if [ -f "$lib" ] ; then
+            echo "$lib"
+          fi
+        done


I was thinking of something like:

for lib in "$d/"*.{dylib,so} ; do local linkPath="$dynamicLinksDir/$(basename "$lib")" if [ -e "$linkPath" ]; then # link already exists! but maybe that's OK if [ ! -l "$linkPath" ] || [ "$(readlink "$linkPath")" != "$lib" ]; then echo >&2 "error: failed to create symbolic link $linkPath: file exists" exit 1 fi else ln -s "$lib" "$linkPath" fi done

I.e., if the place where we want to create the link already exists, then check that it is already a link and already points exactly where we'd like to point it.

gnprice · 2020-04-03T17:17:56Z

pkgs/development/haskell-modules/generic-builder.nix

+    for lib in $(listDynamicLinks | sort | uniq) ; do
+      ln -s "$lib" "$dynamicLinksDir"
    done


Style nit: I think the ln -s step can best be pushed inside the function. Then this is just a call, like:

makeDynamicLinks

That just keeps the ln -s a little closer to the logic that drives it.

(Also, as is I think the sort | uniq shouldn't be needed, because of the sort -u that's inside the function.)

Thank you for your reviewing.
I will update it and check the test.

The hackage-packages.nix change was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@ad4a70d.

haskellPackages.idris: Fix build (new GHC 8.8 & old megaparsec 7)

Test suite requires database. Disabling tests as is done for mysql backend.

haskellPackages.persistent-postgresql: don't check

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@0123dd8.

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@8c7fc4a.

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@e07c49f.

This is the first step towards unbreaking the tensorflow packages.

ld.gold doesn't play well with musl as is documented in NixOS#49071 and https://sourceware.org/bugzilla/show_bug.cgi?id=23856

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@b4a552b.

…skell-1 Unbreak tensorflow haskell 1

ghc: do not use ld.gold with musl libc NixOS#84670

haskellPackages.hsexif: unbroken (fix

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@9501e1f.

ghc: mention why ld.gold is disabled for musl libc

junjihashimoto · 2020-04-11T07:24:28Z

I left one comment above.

Also, please rebase this on the haskell-updates branch.

I rebase this on the haskell-updates branch, but conflicts happen.
Is it right?

cdepillabout · 2020-04-11T07:40:42Z

@junjihashimoto

You'll need to change the base branch on this PR to be haskell-updates, not master.

nh2 · 2020-04-11T13:09:34Z

@junjihashimoto By the way, you do not need to make a new PR to change branches, you can click the Edit button here for that:

nh2 · 2020-04-11T13:10:02Z

For ticket subscribers, new PR is in #84985

junjihashimoto · 2020-04-11T13:26:00Z

Thank you for letting me know.

junjihashimoto requested a review from cdepillabout as a code owner March 28, 2020 16:03

junjihashimoto changed the title ~~Ingnore error of synmlink~~ Ingnore error of symlink Mar 28, 2020

junjihashimoto changed the title ~~Ingnore error of symlink~~ Ignore error of symlink Mar 28, 2020

junjihashimoto force-pushed the feature/symlink-macos branch from a2ad370 to c0b4fd9 Compare March 28, 2020 16:06

junjihashimoto mentioned this pull request Mar 28, 2020

Support for LTS15 of stackage and ghc88 hasktorch/hasktorch#282

Merged

junjihashimoto commented Apr 3, 2020

View reviewed changes

cdepillabout reviewed Apr 3, 2020

View reviewed changes

cdepillabout requested changes Apr 3, 2020

View reviewed changes

gnprice reviewed Apr 3, 2020

View reviewed changes

chkno and others added 12 commits April 3, 2020 14:23

haskellPackages.idris: Fix build (new GHC 8.8 & old megaparsec 7)

645e034

The hackage-packages.nix change was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@ad4a70d.

Merge pull request NixOS#84131 from chkno/idris

55ef84d

haskellPackages.idris: Fix build (new GHC 8.8 & old megaparsec 7)

haskellPackages.persistent-postgresql: don't check

69996c8

Test suite requires database. Disabling tests as is done for mysql backend.

Merge pull request NixOS#84272 from turboMaCk/persistent-postgresql

b0caf86

haskellPackages.persistent-postgresql: don't check

hackage-packages.nix: automatic Haskell package set update

75ae5f3

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@0123dd8.

LTS Haskell 15.7

3b91a24

hackage-packages.nix: automatic Haskell package set update

d85c08d

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@8c7fc4a.

hackage-packages.nix: automatic Haskell package set update

7cf095c

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@e07c49f.

haskellPackages.llvm-hs: Use LLVM 9

8e87b4a

haskellPackages.tensorflow: Update dependencies

88578ec

This is the first step towards unbreaking the tensorflow packages.

ghc: don't use ld.gold with musl libc (fixes NixOS#84670)

acfccf7

ld.gold doesn't play well with musl as is documented in NixOS#49071 and https://sourceware.org/bugzilla/show_bug.cgi?id=23856

hackage-packages.nix: automatic Haskell package set update

88d8977

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@b4a552b.

cdepillabout and others added 10 commits April 9, 2020 10:45

Merge pull request NixOS#84720 from mikesperber/unbreak-tensorflow-ha…

7029177

…skell-1 Unbreak tensorflow haskell 1

Merge pull request NixOS#84741 from adamse/haskell-updates

fcd35ef

ghc: do not use ld.gold with musl libc NixOS#84670

haskellPackages.hsexif: unbroken

29e7274

Merge pull request NixOS#84782 from samdoshi/hsexif

2b9dc72

haskellPackages.hsexif: unbroken (fix

ghc: mention why ld.gold is disabled for musl libc

d7074d3

hackage-packages.nix: automatic Haskell package set update

47e4077

This update was generated by hackage2nix v2.15.1 from Hackage revision commercialhaskell/all-cabal-hashes@9501e1f.

Merge pull request NixOS#84809 from adamse/haskell-updates

5558dc2

ghc: mention why ld.gold is disabled for musl libc

Ignore error of symlink

a9dfd08

Remove duplicated links

0aa4ec4

Add makeDynamicLinks and its comments

e3fb3b0

junjihashimoto force-pushed the feature/symlink-macos branch from 0e3de22 to e3fb3b0 Compare April 11, 2020 07:02

junjihashimoto requested a review from infinisil as a code owner April 11, 2020 07:02

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 11, 2020

junjihashimoto closed this Apr 11, 2020

junjihashimoto mentioned this pull request Apr 11, 2020

Fix the error of making symlink in setupCompilerEnvironmentPhase. #84985

Closed

10 tasks

Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023

Uh oh!

Conversation

junjihashimoto commented Mar 28, 2020

Motivation for this change

Things done

Uh oh!

cdepillabout commented Mar 29, 2020

Uh oh!

nh2 commented Mar 29, 2020

Uh oh!

junjihashimoto commented Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Apr 2, 2020

Uh oh!

junjihashimoto Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

junjihashimoto commented Apr 3, 2020

Uh oh!

cdepillabout Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

cdepillabout left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junjihashimoto Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

junjihashimoto commented Apr 11, 2020

Uh oh!

cdepillabout commented Apr 11, 2020

Uh oh!

nh2 commented Apr 11, 2020

Uh oh!

nh2 commented Apr 11, 2020

Uh oh!

junjihashimoto commented Apr 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

junjihashimoto commented Mar 29, 2020 •

edited

Loading

gnprice Apr 3, 2020 •

edited

Loading