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

Mac compatibility: Avoid PCRE grep in recon-surf, stat formatting and mapfile #672

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

dkuegler
Copy link
Member

@dkuegler dkuegler commented Mar 7, 2025

Avoid using grep -P, instead use grep -E and some bash instructions.

This PR addresses #668 and was introduced by #652.

Needs testing to see if grep 2.6 / mac versions of grep on mac support the extended grep syntax.

This PR also addresses 2 more Mac compatibility issues:

  • The use of mapfile in run_fastsurfer and brun_fastsurfer (the latter will just not be compatible for some specific flags/arguments)
  • the use of stat which uses -c on Linux (GNU coreutils version) and -f for similar things on macOS (BSD version)

Avoid using grep -P, instead use grep -E and some bash instructions.
@dkuegler dkuegler force-pushed the fix/recon-surf-mac branch from 49dd51c to 595f3c9 Compare March 10, 2025 15:14
@m-reuter
Copy link
Member

This works on my Mac. I am getting a print out
A min-conformedconformed image ... which looks weird.

Also I wonder if we should use this PR to fix the other Mac issues:

  • the BSD tool stat in Mac does not have the -c flag (I think it uses -f flag for format)
  • the missing mapfile

@dkuegler
Copy link
Member Author

This works on my Mac. I am getting a print out A min-conformedconformed image ... which looks weird.

Also I wonder if we should use this PR to fix the other Mac issues:

* the BSD tool `stat` in Mac does not have the -c flag (I think it uses -f flag for format)

* the missing `mapfile`

Let's use this issue for that. The grep issue I was fixing here is a mac-only issue.

I will fix the mapfile thing very quickly (this is just an information things, so no damage, just going to skip that for old bash versions).
I will see about stat as well

@dkuegler
Copy link
Member Author

Still working on stat
mapfile changes are untested on macos

@dkuegler dkuegler changed the title Avoid PCRE grep in recon-surf Mac compatibility: Avoid PCRE grep in recon-surf, stat formatting and mapfile Mar 10, 2025
@m-reuter
Copy link
Member

m-reuter commented Mar 11, 2025

I tested mapfile changes and it works (also tested --threads 4 for parallel).

One other thing, I get the run time only in full hours : #@#%# recon-surf-run-time-hours 0.000

Probably due to first output line: time command failing, not using time...
while a time command is available on Mac, probably uses a different format.
Update: I just saw that on Linux it also only reports the full hours. So we should probably report also minutes in both cases and we should check if the time command failing printout should be changed to a message that looks less like an error or warning, maybe "unexpected time command (maybe on MAC), using ??? instead"

@dkuegler dkuegler force-pushed the fix/recon-surf-mac branch from 18a20a7 to 20b009d Compare March 11, 2025 17:11
@dkuegler dkuegler force-pushed the fix/recon-surf-mac branch from 20b009d to 2db6160 Compare March 12, 2025 18:44
@m-reuter
Copy link
Member

I still get ./FastSurfer/run_fastsurfer.sh: line 828: mapfile: command not found. Running a test now.

@dkuegler
Copy link
Member Author

I still get ./FastSurfer/run_fastsurfer.sh: line 828: mapfile: command not found. Running a test now.

What do you get, if you run bash --version ?

@m-reuter
Copy link
Member

inside the scripts it uses the system bash, which is this:

> /bin/bash --version
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin24)
Copyright (C) 2007 Free Software Foundation, Inc.

in my terminal I have a newer one (5.something) via home-brew, but system bash is still old, and that is the one that gets used via the bash shebang.

@dkuegler
Copy link
Member Author

So the difference is that when we call bash --version, it is different to ./reconsurf.sh which uses the other bash .... This is so confusing.

@m-reuter
Copy link
Member

it can be different depending on what other bash is installed on the system and in the path. On normal systems, it should be the same.

…les) into a function in functions.

This check uses `stat`, which works in different ways for macOS and linux.
Make the function (stat) work for both linux (GNU coreutils) and macOS (unclear version).
use bc -l flag to use fractions as well
@dkuegler dkuegler force-pushed the fix/recon-surf-mac branch from 2db6160 to 103fef9 Compare March 13, 2025 10:11
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.

2 participants