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

Do not export rlocation from test setup #24256

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Nov 8, 2024

The rlocation implementation lacks many of the features of the shell runfiles library, which should be dependend on explicitly instead.

When called, rlocation will now fail with an error message.

The `rlocation` implementation lacks many of the features of the shell runfiles library, which should be dependend on explicitly instead.

When called, `rlocation` will now fail with an error message.
@fmeum fmeum requested a review from meteorcloudy November 8, 2024 13:44
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 8, 2024

@bazel-io fork 8.0.0

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Nov 8, 2024
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@fmeum fmeum requested a review from Wyverald November 8, 2024 13:53
@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 8, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 11, 2024
@fmeum fmeum deleted the test-setup-rlocation branch November 11, 2024 11:21
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Nov 11, 2024
The `rlocation` implementation lacks many of the features of the shell runfiles library, which should be dependend on explicitly instead.

When called, `rlocation` will now fail with an error message.

Closes bazelbuild#24256.

PiperOrigin-RevId: 695241586
Change-Id: I7059c329fd6ca518623d7be8ad3514082097c191
github-merge-queue bot pushed a commit that referenced this pull request Nov 12, 2024
The `rlocation` implementation lacks many of the features of the shell
runfiles library, which should be dependend on explicitly instead.

When called, `rlocation` will now fail with an error message.

Closes #24256.

PiperOrigin-RevId: 695241586
Change-Id: I7059c329fd6ca518623d7be8ad3514082097c191

Commit
1312007

Co-authored-by: Fabian Meumertzheim <[email protected]>
@@ -236,6 +234,15 @@ else
TEST_PATH="$(rlocation $TEST_WORKSPACE/$EXE)"
fi

# Redefine rlocation to notify users of its removal - it used to be exported.
# TODO: Remove this before Bazel 9.
function rlocation() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be compatible with /bin/sh which caused a test failure at https://buildkite.com/bazel/google-bazel-presubmit/builds/87576#0193afe3-eb2a-49df-b687-13d1be1ce0f7

I can change #!/bin/sh to #!/usr/bin/env bash to fix it in pywrap_template.txt, but maybe we should try to make it compatible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function rlocation() {
  caller 0 | {
    read LINE SUB FILE
    echo >&2 "ERROR: rlocation is no longer implicitly provided by Bazel's test setup, but called from $SUB in line $LINE of $FILE. Please use https://github.com/bazelbuild/rules_shell/blob/main/shell/runfiles/runfiles.bash instead."
    exit 1
  }
}

seems to work

copybara-service bot pushed a commit that referenced this pull request Dec 10, 2024
Addressing #24256 (comment)

PiperOrigin-RevId: 704624513
Change-Id: I73ec6559a7a7b2465abd2faae0e687a97ea77db9
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 10, 2024
Addressing bazelbuild#24256 (comment)

PiperOrigin-RevId: 704624513
Change-Id: I73ec6559a7a7b2465abd2faae0e687a97ea77db9
github-merge-queue bot pushed a commit that referenced this pull request Dec 11, 2024
Addressing
#24256 (comment)

PiperOrigin-RevId: 704624513
Change-Id: I73ec6559a7a7b2465abd2faae0e687a97ea77db9

Commit
9ed06ad

Co-authored-by: Googler <[email protected]>
copybara-service bot pushed a commit that referenced this pull request Dec 11, 2024
pywrapper_template.txt change was due to #24256 (comment)

PiperOrigin-RevId: 705079774
Change-Id: I4acbce5096b87e30add5cba1632fae1eab7627c0
ramil-bitrise pushed a commit to bitrise-io/bazel that referenced this pull request Dec 18, 2024
The `rlocation` implementation lacks many of the features of the shell runfiles library, which should be dependend on explicitly instead.

When called, `rlocation` will now fail with an error message.

Closes bazelbuild#24256.

PiperOrigin-RevId: 695241586
Change-Id: I7059c329fd6ca518623d7be8ad3514082097c191
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