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

relativize path for scalac classpath entry #8011

Merged
merged 1 commit into from Jul 4, 2019

Conversation

Projects
None yet
4 participants
@ity
Copy link
Contributor

commented Jul 3, 2019

Problem

When using remote hermetic zinc (without native-image) we were encountering an error where a path was not found during remote execution. Digging into it, it was obvious that the path being sent wasnt being rewritten/relativized.

Solution

added relativization to all classpath entries for scalac, confirmed this fixes the issue locally.

@ity ity added the needs-cherrypick label Jul 3, 2019

@ity ity requested a review from stuhood Jul 3, 2019

@stuhood

stuhood approved these changes Jul 4, 2019

Copy link
Member

left a comment

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

If we're going to rely on spinning up scoot or something for integration testing here, we should think about doing so in a docker container, to avoid absolute paths happening to be present on the machine :)

@illicitonion illicitonion merged commit 2991f14 into pantsbuild:master Jul 4, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@ity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

If we're going to rely on spinning up scoot or something for integration testing here, we should think about doing so in a docker container, to avoid absolute paths happening to be present on the machine :)

thats sounds great to me!

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Thanks @ity ! This code is... hard to test, unfortunately. We need to get an end-to-end remoting test working, possibly via #7553, but until then: fine as is.

If we're going to rely on spinning up scoot or something for integration testing here, we should think about doing so in a docker container, to avoid absolute paths happening to be present on the machine :)

Mm, good point. Or maybe explicitly using RBE from an integration test will be reasonable sometime soon. cc @Eric-Arellano

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

I don’t think remoting an integration test will happen soon, unfortunately, due to starting teaching soon. Unless we expect it to be trivial to remote integration tests after we remote unit tests, which I don’t anticipate.

Unit tests should be soon! It seems all we need is to finish this token generator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.