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

FIX use --transform in freesurfer's template to generalize across freesurfer versions to strip leading freesurfer/ folder #626

Merged
merged 2 commits into from
Jul 6, 2024

Conversation

mvdoc
Copy link
Member

@mvdoc mvdoc commented Jun 22, 2024

I guess we'll have to check if the same problem occurs with new freesurfer versions.

Closes #625

@yarikoptic
Copy link
Member

could you check newer ones or they aren't out? or may be --transform= which takes regex to change filepaths could be used for more flexibility?

@mvdoc
Copy link
Member Author

mvdoc commented Jul 5, 2024

Ah good ol' Yarik always teaches me something new! I switched to --transform and it works great

7.3.2

$ neurodocker generate docker --base-image ubuntu:20.04 --pkg-manager apt --freesurfer version=7.3.2 > test-dockerfile-7.3.2
$ docker build --file test-dockerfile-7.3.2 . --tag freesurfer-7.3.2
$ docker run --rm -it freesurfer-7.3.2:latest ls /opt/freesurfer-7.3.2 && cat /opt/freesurfer-7.3.2/b
uild-stamp.txt
ASegStatsLUT.txt               average          matlab
DefectLUT.txt                  bin              mni
FreeSurferColorLUT.txt         build-stamp.txt  mni-1.4
FreeSurferEnv.csh              diffusion        models
FreeSurferEnv.sh               docs             python
SegmentNoLUT.txt               etc              sessions
SetUpFreeSurfer.csh            freesurfer       sources.csh
SetUpFreeSurfer.sh             fsafd            sources.sh
Simple_surface_labels2009.txt  fsfast           subjects
SubCorticalMassLUT.txt         lib              tkmeditParcColorsCMA
WMParcStatsLUT.txt             luts             tktools

$ docker run --rm -it freesurfer-7.3.2:latest cat /opt/freesurfer-7.3.2/build-stamp.txt
freesurfer-linux-centos7_x86_64-7.3.2-20220804-6354275

7.4.1

$ neurodocker generate docker --base-image ubuntu:20.04 --pkg-manager apt --freesurfer version=7.4.1 > test-dockerfile-7.4.1
$ docker build --file test-dockerfile-7.4.1 . --tag freesurfer-7.4.1
$ docker run --rm -it freesurfer-7.4.1:latest ls /opt/freesurfer-7.4.1 
ASegStatsLUT.txt               average          matlab
DefectLUT.txt                  bin              mni
FreeSurferColorLUT.txt         build-stamp.txt  mni-1.4
FreeSurferEnv.csh              diffusion        models
FreeSurferEnv.sh               docs             python
SegmentNoLUT.txt               etc              sessions
SetUpFreeSurfer.csh            freesurfer       sources.csh
SetUpFreeSurfer.sh             fsafd            sources.sh
Simple_surface_labels2009.txt  fsfast           subjects
SubCorticalMassLUT.txt         lib              tkmeditParcColorsCMA
WMParcStatsLUT.txt             luts             tktools

$ docker run --rm -it freesurfer-7.4.1:latest cat /opt/freesurfer-7.4.1/build-stamp.txt
freesurfer-linux-centos7_x86_64-7.4.1-20230613-7eb8460

@mvdoc mvdoc changed the title FIX strip-components 2 for freesurfer 7.4.1 FIX use --transform in freesurfer's template to generalize across freesurfer versions Jul 5, 2024
@yarikoptic
Copy link
Member

yarikoptic commented Jul 6, 2024

Ah good ol' Yarik

That elderly learns something new each day too ;-) I eye balled failing tests and it is still only the two of test_build_image_from_registered on centos, so unrelated. Leaving a comment on the diff now... nah -- I think we are all good -- for better or for worse -- that --transform works great.

@yarikoptic yarikoptic added release to trigger release from pr patch patch tag for release labels Jul 6, 2024
@yarikoptic yarikoptic changed the title FIX use --transform in freesurfer's template to generalize across freesurfer versions FIX use --transform in freesurfer's template to generalize across freesurfer versions to strip leading freesurfer/ folder Jul 6, 2024
@yarikoptic yarikoptic merged commit ab03622 into ReproNim:master Jul 6, 2024
7 of 12 checks passed
@yarikoptic
Copy link
Member

interestingly, even in 7.3.2 there were some ./freesurfer entries!

$> tar -tvf freesurfer-linux-centos7_x86_64-7.3.2.tar | awk '{print $6;}' | sed -e 's,^\([^/]*/[^/]*\).*,\1,g' | uniq -c
      1 freesurfer/
     11 freesurfer/tktools
    784 freesurfer/fsfast
    474 freesurfer/mni-1.4
      1 freesurfer/FreeSurferEnv.csh
      1 freesurfer/FreeSurferEnv.sh
   1665 freesurfer/trctrain
      1 freesurfer/SetUpFreeSurfer.csh
    113 freesurfer/docs
     37 freesurfer/models
    450 freesurfer/lib
     94 freesurfer/diffusion
      1 freesurfer/SegmentNoLUT.txt
  42431 freesurfer/python
      1 freesurfer/build-stamp.txt
      4 freesurfer/fsafd
      1 freesurfer/sources.csh
      1 freesurfer/DefectLUT.txt
    478 freesurfer/mni
   1954 freesurfer/subjects
      1 freesurfer/FreeSurferColorLUT.txt
      1 freesurfer/ASegStatsLUT.txt
      1 freesurfer/SubCorticalMassLUT.txt
    795 freesurfer/bin
      1 freesurfer/Simple_surface_labels2009.txt
      1 freesurfer/tkmeditParcColorsCMA
    287 freesurfer/matlab
      1 freesurfer/WMParcStatsLUT.txt
      1 freesurfer/sources.sh
   7575 freesurfer/average
      2 freesurfer/etc
      1 freesurfer/SetUpFreeSurfer.sh
      5 freesurfer/luts
      2 freesurfer/sessions
    953 ./freesurfer
$> tar -tvf freesurfer-linux-centos7_x86_64-7.3.2.tar | awk '{print $6;}' | grep '^\./freesurfer' | head                        
./freesurfer/lib/qt/
./freesurfer/lib/qt/lib/
./freesurfer/lib/qt/lib/libQt5Network.so
./freesurfer/lib/qt/lib/libQt5Gamepad.so.5.12

I assume that when extracting without stripping it worked just fine. So could it be we might have fixed more than planned? do you have still those examples from prior version to see if lib/qt was extracted under freesurfer/lib/qt ?

@mvdoc
Copy link
Member Author

mvdoc commented Jul 6, 2024

I'm afraid you're correct, the transform now strips too much. Good catch.

With --strip-components=1:

$ docker run --rm -it freesurfer-7.3.2-strip:latest ls -la /opt/freesurfer-7.3.2/freesurfer/
total 12
drwxr-xr-x  3 root root 4096 Jul  6 20:17 .
drwxr-xr-x 20 root root 4096 Jul  6 20:17 ..
drwxr-xr-x  3 root root 4096 Jul  6 20:17 lib

But with the --transform it is now empty:

$ docker run --rm -it freesurfer-7.3.2:latest ls -la /opt/freesurfer-7.3.2/freesurfer/
total 8
drwxrwxr-x  2 root root 4096 Aug  4  2022 .
drwxr-xr-x 20 root root 4096 Jul  5 22:27 ..

However I don't know if those libs are necessary, since we already exclude lib/qt and lib/cuda from the extraction process. The only remaining libs are for tktools:

$ docker run --rm -it freesurfer-7.3.2-strip:latest ls -la /opt/freesurfer-7.3.2/freesurfer/lib/
total 12
drwxr-xr-x 3 root root 4096 Jul  6 20:17 .
drwxr-xr-x 3 root root 4096 Jul  6 20:17 ..
drwxrwxr-x 7 root root 4096 Mar 30  2020 tktools

@yarikoptic
Copy link
Member

the transform now strips too much.

I would not say "now strips too much" but rather "now strips the correct amount" since without stripping there is no difference between freesurfer/ and ./freesurfer/, correct? And because you didn't use g modifier, it strips only leading freesurfer and not any other possible freesurfer subfolder, thus making it "correct". Or am I logic flawed?

@yarikoptic
Copy link
Member

we only could have made it more precise may be anchoring like 's,^\(\./\)*freesurfer/,,' and more robust by adding version information, smth like -e 's,^\(\./\)*freesurfer\(-[^/]*\)/,,' in case if any release packages it that way. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch patch tag for release release to trigger release from pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Freesurfer 7.4.1 installation requires --strip-components 2 instead of 1 when installing
2 participants