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

Implement repository_ctx.read() for reading local files. #7309

Closed
wants to merge 4 commits into from
Closed

Implement repository_ctx.read() for reading local files. #7309

wants to merge 4 commits into from

Conversation

jmillikin
Copy link
Contributor

Fixes #3766

@jmillikin jmillikin mentioned this pull request Jan 31, 2019
Closed
@dslomov
Copy link
Contributor

dslomov commented Jan 31, 2019

Let's discuss the use-case for this in #3766.

@laurentlb laurentlb added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. and removed team-Starlark labels Feb 5, 2019
@jmillikin-stripe
Copy link
Contributor

@dslomov: ping

dslomov
dslomov suggested changes Mar 8, 2019
View reviewed changes
Copy link
Contributor

@dslomov dslomov left a comment

Choose a reason for hiding this comment

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

We should completely remove encoding parameter from readFile

...in/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java Outdated Show resolved Hide resolved
@dslomov
Copy link
Contributor

dslomov commented Mar 8, 2019

@c-parsons please take a look from Starlark API perspective

@jmillikin
Copy link
Contributor Author

(rebased to current master HEAD)

@jin jin mentioned this pull request Mar 11, 2019
Closed
@jmillikin
Copy link
Contributor Author

Latest commit addresses review feedback. PTAL.

@jmillikin jmillikin changed the title Implement repository_ctx.read() for reading local files. Implement repository_ctx.read_file() for reading local files. Mar 12, 2019
dslomov
dslomov suggested changes Mar 12, 2019
View reviewed changes
Copy link
Contributor

@dslomov dslomov left a comment

Choose a reason for hiding this comment

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

(see comments in conversation)

@jmillikin jmillikin changed the title Implement repository_ctx.read_file() for reading local files. Implement repository_ctx.read() for reading local files. Mar 13, 2019
@jmillikin
Copy link
Contributor Author

CI is failing due to what looks like a bazel mirror outage.

dslomov
dslomov suggested changes Mar 13, 2019
View reviewed changes
Copy link
Contributor

@dslomov dslomov left a comment

Choose a reason for hiding this comment

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

(see above for comments)

dslomov
dslomov approved these changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@dslomov dslomov left a comment

Choose a reason for hiding this comment

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

Thank you!

dslomov
dslomov suggested changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@dslomov dslomov left a comment

Choose a reason for hiding this comment

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

CI failures look relevant?

@jmillikin
Copy link
Contributor Author

Thanks, didn't see that test earlier. I fixed up the calls to createFile() for the new signature, and also added a test of readFile().

It passes locally. BuildKite CI seems fairly slow, so if it goes red again then I'll try again tonight.

@jmillikin
Copy link
Contributor Author

@dslomov Tests are green.

@dslomov dslomov requested a review from brandjon March 14, 2019 17:59
@dslomov
Copy link
Contributor

dslomov commented Mar 14, 2019

@brandjon had stylistic comments on the internal review - please review this PR.

@dslomov dslomov assigned brandjon and unassigned dslomov Mar 14, 2019
brandjon
brandjon requested changes Mar 14, 2019
View reviewed changes
Copy link
Member

@brandjon brandjon left a comment

Choose a reason for hiding this comment

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

I'm no expert on the specific code being touched by this PR, but I have some readability concerns.

...in/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java Show resolved Hide resolved
src/test/shell/bazel/bazel_workspaces_test.sh Show resolved Hide resolved
@bazel-io bazel-io closed this in b11203d Mar 14, 2019
@pauldraper pauldraper mentioned this pull request Oct 14, 2019
Open
@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
@philwo philwo removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgruber cgruber cgruber left review comments

@jmillikin-stripe jmillikin-stripe jmillikin-stripe left review comments

@brandjon brandjon brandjon approved these changes

@dslomov dslomov dslomov approved these changes

@c-parsons c-parsons Awaiting requested review from c-parsons

Assignees

@brandjon brandjon

Labels
cla: yes team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Please add a repository_ctx method for reading files by Label
8 participants
@jmillikin @dslomov @jmillikin-stripe @cgruber @brandjon @laurentlb @philwo @googlebot