-
-
Notifications
You must be signed in to change notification settings - Fork 287
Make DepsTrackingReporter handle Windows path separators #1503
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
Make DepsTrackingReporter handle Windows path separators #1503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @crt-31, LGTM, please address the comment
@@ -85,12 +85,16 @@ def _write_executable_windows(ctx, executable, rjars, main_class, jvm_flags, wra | |||
cpfile = ctx.actions.declare_file("%s.classpath" % ctx.label.name) | |||
ctx.actions.write(cpfile, classpath) | |||
|
|||
specifiedEnv = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the same change in both PRs? #1502 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both PRs need the change to work (I was trying to make the PRs standalone). But I could put in only one if thats what you want, probably not big deal if you are gonna commit both PRs soon.
@liucijus please advise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I removed this 'specifiedEnv' change from this PR.
@liucijus Ready for your review.
927cfd5
to
5093801
Compare
@@ -17,6 +17,9 @@ | |||
import java.util.Set; | |||
import java.util.jar.JarFile; | |||
import java.util.stream.Collectors; | |||
|
|||
import javax.print.attribute.standard.Severity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to be unused import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liucijus: ok. That unused import is removed.
Fixed.
@liucijus, I'm not sure why the CI tests failed on this merge. The tests ran fine on my linux vm. |
Description
This fixes issue where the ‘compiler-dependency-analyzer’ was not handling Windows path separator and failed with false negatives.
Motivation
Makes the new compiler-dependency-analyzer work on Windows.
Before this PR, on Windows, compiling would report a bunch of erroneous 'unused deps' errors.
This fixes #1487