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

Only process executable path for path mapping #24576

Closed

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 5, 2024

This fixes a few issues introduced in 282ac62:

  • Only the executable path in the first argument of a CommandLines instance should be subject to path mapping.
  • String arguments that are not normalized as path strings and thus can't possibly be the optimized memory representation used for the executable arg by StarlarkAction should not be modified.

Also removes an class that became unused in that commit.

This fixes a few issues introduced in 282ac62:
* Only the executable path in the first argument of a `CommandLines` instance should be subject to path mapping.
* String arguments that are not normalized as path strings and thus can't possibly be the optimized memory representation used for the executable arg by `StarlarkAction` should not be modified.

Also removes an class that became unused in that commit.
@fmeum fmeum requested a review from justinhorvitz December 5, 2024 11:37
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 5, 2024

@bazel-io flag

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 5, 2024

(for 8.0.1 or 8.1.0)

@github-actions github-actions bot added team-Performance Issues for Performance teams awaiting-review PR is awaiting review from an assigned reviewer labels Dec 5, 2024
@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 5, 2024
@fmeum fmeum requested a review from justinhorvitz December 6, 2024 16:53
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 6, 2024

@meteorcloudy If this makes it in time for the next 8.0.0 RC, it would be safer to include it.

@copybara-service copybara-service bot closed this in b00576d Dec 6, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Dec 6, 2024
@fmeum fmeum deleted the 24546-path-mapped-executable-fix branch December 6, 2024 18:26
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 6, 2024

@bazel-io fork 8.0.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 6, 2024
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 6, 2024
This fixes a few issues introduced in 282ac62:
* Only the executable path in the first argument of a `CommandLines` instance should be subject to path mapping.
* String arguments that are not normalized as path strings and thus can't possibly be the optimized memory representation used for the executable arg by `StarlarkAction` should not be modified.

Also removes an class that became unused in that commit.

Closes bazelbuild#24576.

PiperOrigin-RevId: 703536848
Change-Id: I358696bc268ab1f400798ef55ea4db8d556f07dd
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2024
This fixes a few issues introduced in
282ac62:
* Only the executable path in the first argument of a `CommandLines`
instance should be subject to path mapping.
* String arguments that are not normalized as path strings and thus
can't possibly be the optimized memory representation used for the
executable arg by `StarlarkAction` should not be modified.

Also removes an class that became unused in that commit.

Closes #24576.

PiperOrigin-RevId: 703536848
Change-Id: I358696bc268ab1f400798ef55ea4db8d556f07dd

Commit
b00576d

Co-authored-by: Fabian Meumertzheim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Performance Issues for Performance teams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants