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

[chore] Fix make update-otel on macOS #35587

Merged

Conversation

jade-guiton-dd
Copy link
Contributor

Description:

When working on #35580, it seemed that make update-otel was not properly updating the builder-config.yaml files. I traced this down to the updatehelper script not working as intended. This turned out to be because it uses some features of the sed utility which do not exist on macOS, causing no updates to be made.

The \s (whitespace) regex class is a GNU extension for sed; the POSIX-compatible equivalent is [[:space:]] (related SO question).

Moreover, on macOS, the -i (in-place) option requires an argument, even if empty; otherwise the -e following it is parsed as that argument, creating an unnecessary backup file (related SO question).

Testing:

I manually tested this change as part of developing the aforementioned PR; the script seems to work on macOS now. It would be good for someone to test this change on Linux to make sure nothing has broken there.

@dmitryax
Copy link
Member

dmitryax commented Oct 5, 2024

@jade-guiton-dd, can you please validate it on a linux VM as well? I think everyone runs MacOS locally. Someone has to spin up a linux VM :)

@jade-guiton-dd
Copy link
Contributor Author

jade-guiton-dd commented Oct 8, 2024

After testing on a Linux device, it looks like the \s change works fine. The -i change also works, but prints a lot of error messages (sed: can't read : No such file or directory, because the '' is interpreted as a second input file). After actually reading the whole SO thread I linked above, it seems like there is no clean portable way of using sed in-place without creating backup file... I'll modify the PR to create a backup file and immediately delete it, which is a somewhat janky solution, but it works.

The `\s` (whitespace) regex class is a GNU extension for `sed`; the POSIX-compatible equivalent is `[[:space:]]`. Moreover, the `-i` (in-place) option requires an argument, even if empty; otherwise the `-e` following it is parsed as that argument.
@MovieStoreGuy MovieStoreGuy merged commit 7d92d6a into open-telemetry:main Oct 22, 2024
155 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 22, 2024
@jade-guiton-dd jade-guiton-dd deleted the fix-update-otel-macos branch November 27, 2024 13:38
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.

4 participants