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

Bazel remote execution support for a symlink to $JAVA_HOME #391

Merged
merged 8 commits into from Aug 31, 2018

Conversation

Projects
None yet
3 participants
@ryancouto
Copy link
Member

ryancouto commented Aug 29, 2018

No description provided.

Show resolved Hide resolved common/common.go
log.Error(msg)
return nil, fmt.Errorf(msg)
}
if b, err := exec.Command("ln", "-s", javaHome, platProp.GetValue()).CombinedOutput(); err != nil {

This comment has been minimized.

@dgassaway

dgassaway Aug 30, 2018

Contributor

Let's use https://golang.org/pkg/os/#Symlink instead of exec here

if platProp.GetName() == "JDK_SYMLINK" {
javaHome, ok := os.LookupEnv("JAVA_HOME")
if !ok {
msg := "Unsuccessful lookup of $JAVA_HOME, not defined."

This comment has been minimized.

@dgassaway

dgassaway Aug 30, 2018

Contributor

Suggest more generic prefix for errors here - "Failed setting up platform property X: $reason"

Show resolved Hide resolved runner/runners/bazel.go Outdated

illicitonion added a commit to twitter/pants that referenced this pull request Aug 30, 2018

Set JDK properties for remote execution
As implemented in twitter/scoot#391 on the
server-side

illicitonion added a commit to twitter/pants that referenced this pull request Aug 30, 2018

Set JDK properties for remote execution
As implemented in twitter/scoot#391 on the
server-side

illicitonion added a commit to pantsbuild/pants that referenced this pull request Aug 30, 2018

Set JDK properties for remote execution (#6417)
As implemented in twitter/scoot#391 on the
server-side
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 30, 2018

Codecov Report

Merging #391 into master will decrease coverage by 0.08%.
The diff coverage is 18.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
- Coverage   56.63%   56.54%   -0.09%     
==========================================
  Files         106      106              
  Lines        7775     7789      +14     
==========================================
+ Hits         4403     4404       +1     
- Misses       2934     2947      +13     
  Partials      438      438
Impacted Files Coverage Δ
snapshot/bazel/filer.go 64.7% <ø> (ø) ⬆️
runner/runners/bazel.go 0% <0%> (ø) ⬆️
snapshot/bazel/checkouter.go 51.51% <100%> (ø) ⬆️
runner/runners/invoke.go 57.1% <20%> (-1.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4316764...cb59c28. Read the comment docs.

bzFiler.JDKSymlinkCh <- platProp.GetValue()
break
}
bzFiler.JDKSymlinkCh <- nil

This comment has been minimized.

@dgassaway

dgassaway Aug 30, 2018

Contributor

I think this will not work if we get multiple properties and the first one is not "JDK_SYMLINK" - the if will get skipped and we'll send nil on the ch, even if the next prop was "JDK_SYMLINK."

This looks pretty dangerous being async - if there is ever a call to checkout without a channel send we will deadlock. I was thinking we would just write the symlink dir ontop of the checkout path after it's done (using the original logic, no change to the filer required).

DPT-11882 is more about how we could do better support for this type of thing vs throwing logic into runners/ - maybe an additional CheckoutExtended function in the Checkouter interface would add flexibility.

This comment has been minimized.

@ryancouto

ryancouto Aug 31, 2018

Member

Misunderstood your previous feedback. Submitting new diff...

ryancouto added some commits Aug 31, 2018

@dgassaway
Copy link
Contributor

dgassaway left a comment

Looks good! Sorry about the confusion from my first request.

@ryancouto ryancouto merged commit 78f93a8 into master Aug 31, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@ryancouto ryancouto deleted the rcouto/dpt-12395/plat-props branch Aug 31, 2018

ity added a commit to ity/pants that referenced this pull request Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment