-
Notifications
You must be signed in to change notification settings - Fork 221
enh: replace unix2dos with sed #3210
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
enh: replace unix2dos with sed #3210
Conversation
/intelci: run |
/intelci: run |
@@ -1040,7 +1040,7 @@ $4: $3/$2 | |||
$3/$2: $(DIR)/$1 | $3/. ; $(value cpy) | |||
$(if $(filter %.sh %.bat dal,$2),sed -i -e 's/__DAL_MAJOR_BINARY__/$(MAJORBINARY)/' $3/$2) | |||
$(if $(filter %.sh %.bat dal,$2),sed -i -e 's/__DAL_MINOR_BINARY__/$(MINORBINARY)/' $3/$2) | |||
$(if $(OS_is_win),unix2dos $3/$2) | |||
$(if $(OS_is_win),sed -i -n -z -e 's/\r*\n/\r\n/g;p' $3/$2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to worry about it has to work on other systems like macos(some of the sed arg is not supported I think) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OneDAL currently does not support macOS so no need to worry about it.
Sed is part of most coreutils out there (like GNUs) and is required by POSIX, so even if it doesn't come out of the box on macOS (although I'm pretty sure it does), it can be installed easily afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think I misread it: yes, some arguments might indeed be GNU-specific extensions. This needs to be documented.
@Alexandr-Solovev Could you please add a note that this requires the GNU variant of sed if that's the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its implemented only for if $(OS_is_win), so, I think its not necessary to think about other system support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alexandr-Solovev I think it's still theoretically possible to install non-GNU sed variants on windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think msys2 uses GNU sed.
@Alexandr-Solovev Could you please remove the dos2unix requirement and instructions from the INSTALL.md file? |
/intelci: run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scripts contain Windows-style line endings after the change.
Summary
Replaced usage of
unix2dos
withsed
to remove an external dependency.Details
The original script relied on
dos2unix
, which is not always available in all environments by default. This change replaces it with ased
command that achieves the same result (removing carriage return characters), thus improving portability and reducing setup requirements.Changes
unix2dos
unix2dos
utilityMotivation
Minimize external dependencies and ensure better cross-platform compatibility.
PR completeness and readability
Testing
Performance