Smaller Fixes #243
Smaller Fixes #243
Conversation
User-Domme
commented
Nov 4, 2025
User-Domme
commented
- Re-enabled C++17 support (previously forced C++14 for legacy reasons).
- Fixed sed expression that mistakenly removed '-l' inside terms like '-linux'.
- Added PyTorch include/library path detection for Conda environments.
- Fixed sed expression that mistakenly removed '-l' inside terms like '-linux'. - Added PyTorch include/library path detection for Conda environments.
sjanssen2
left a comment
sjanssen2
left a comment
There was a problem hiding this comment.
Thanks a lot. However, I do have comments / questions
| << endl; | ||
| stream << "library_dirs := $(LDFLAGS) | sed -e 's/\\s/\", \"/g'" << endl; | ||
| stream << "LDLIBS := $(LDLIBS) | sed 's/-l//g'" << endl; | ||
| stream << "LDLIBS := $(LDLIBS) | sed -E 's/(^|[[:space:]])-l/\\1/g'" << endl; |
There was a problem hiding this comment.
not sure if this is relevant here, but sed behaves differently in OSX and Linux
Can you also show an example of what sed should change here?
what does the | mean in the regex? why are you capturing multiple spaces?
There was a problem hiding this comment.
The old sed would falsely remove all "-l" within a term for example:
"-l/usr/lib/x86_64-linux-gnu, -lsd" --> "/usr/lib/x86_64inux-gnu, sd" the "64-linux" turns into "64inux" which causes an error.
The new expression makes sure the "-l" is either at the start of a line (^) or (|) behind a space, line break or tab ( [[:SPACE:]] ). The "\1" is needed to keep the capture group that is detected within the "( )" and not fully replace it, otherwise the space between terms would be removed as well and 2 terms would falsely merge into one.
So finally the new expression correctly performs the following:
"-l/usr/lib/x86_64-linux-gnu, -lsd" --> "/usr/lib/x86_64-linux-gnu, sd"
For OSX the regex should do the same thing, at least i tested it on an OSX terminal as well.
