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.

Sign up for GitHub

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

Jump to bottom

Allow buildifier_test to escape the sandbox #1092

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Allow buildifier_test to escape the sandbox #1092

merged 1 commit into from
Nov 10, 2022

Conversation

milesdai
Copy link
Contributor

This PR addresses #1075 by adding a no_sandbox attribute and a WORKSPACE path attribute tobuildifier_test. Currently, users must explicitly specify every BUILD file they would like to lint in the srcs attribute of buildifier_test. This PR allows the user to create a test to lint all BUILD files as follows:

buildifier_test(
    name = "buildifier_check",
    diff_command = "diff -u",
    mode = "diff",
    no_sandbox = True,
    workspace = "//:WORKSPACE",
)

@milesdai milesdai force-pushed the sandbox-escape branch 2 times, most recently from 084ef79 to 63313d6 Compare October 10, 2022 17:55
@cfrantz
Copy link

cfrantz commented Oct 10, 2022

We (opentitan-team) are curious if structuring global "whole project" tests like this is bad and if there is a better way for the test to find the root subdir of the project (tests don't have BUILD_WORKSPACE_DIRECTORY, so we're using $(dirname $(realpath $WORKSPACE_FILE))).

sfc-gh-ebrossard
sfc-gh-ebrossard reviewed Nov 7, 2022
View reviewed changes
Copy link

@sfc-gh-ebrossard sfc-gh-ebrossard left a comment

Choose a reason for hiding this comment

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

I would love to see this merged as well :)

buildifier/runner.bash.template Outdated Show resolved Hide resolved
buildifier/internal/factory.bzl Outdated Show resolved Hide resolved
vladmos
vladmos approved these changes Nov 9, 2022
View reviewed changes
buildifier/buildifier.bzl Outdated Show resolved Hide resolved
@milesdai
Allow buildifier_test to escape the sandbox
edebdd1 
This allows buildifier_test to access the entire workspace to avoid
needing to explicitly specify which files should be linted.
@vladmos vladmos merged commit 762712d into bazelbuild:master Nov 10, 2022
@milesdai milesdai deleted the sandbox-escape branch November 10, 2022 18:52
@jszumski jszumski mentioned this pull request Jan 24, 2023
Merged
@erikkerber erikkerber mentioned this pull request Mar 29, 2023
Merged
keith pushed a commit to keith/buildifier-prebuilt that referenced this pull request Mar 30, 2023
@erikkerber
Allow buildifier_test to escape the sandbox (#59)
5b6adef 
Port of bazelbuild/buildtools#1092

(Don't love the ergonomics of these rules, but we've already invested in
them I guess)
This was referenced Jul 27, 2023
Open
Open
@hunshcn hunshcn mentioned this pull request Mar 23, 2024
Closed
apattidb pushed a commit to databricks/buildtools that referenced this pull request May 10, 2024
@milesdai @apattidb
Allow buildifier_test to escape the sandbox (bazelbuild#1092)
d818be3 
This allows buildifier_test to access the entire workspace to avoid
needing to explicitly specify which files should be linted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-ebrossard sfc-gh-ebrossard sfc-gh-ebrossard left review comments

@vladmos vladmos vladmos approved these changes

@pmbethe09 pmbethe09 Awaiting requested review from pmbethe09 pmbethe09 is a code owner

@meisterT meisterT Awaiting requested review from meisterT

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@milesdai @cfrantz @vladmos @sfc-gh-ebrossard