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

SourceFileTest.testSourceFileResolvesZipEntries failes for Windows #3206 #3207

Closed
wants to merge 1 commit into from

Conversation

lgemeinhardt
Copy link
Contributor

And fixes parts of "Support the workflow under Windows. #9" (google/j2cl#9) from j2cl

@@ -359,13 +359,13 @@ public String toString() {
private static final String JAR_URL_PREFIX = "jar:file:";

private static boolean isZipEntry(String path) {
return path.contains(".zip!" + File.separatorChar) && (path.endsWith(".js") || path.endsWith(".js.map"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, what's the issue with File.separatorChar in GWT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It simple does not exists in the emulation / super source.

}

@GwtIncompatible("java.io.File")
private static SourceFile fromZipEntry(String zipURL, Charset inputCharset) {
checkArgument(isZipEntry(zipURL));
String[] components = zipURL.split(Pattern.quote(BANG_SLASH.replace('/', File.separatorChar)));
String[] components = zipURL.split(Pattern.quote(BANG_SLASH.replace("/", File.separator)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing a replace, can you fix the BANG_SLASH definition to

private static final String BANG_SLASH = "!" + File.separatorChar;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was also my first idea, but BANG_SLASH is used 3 times, but only one is windows (path) dependend. Only using another constant (like BANG_SLASH_FS_SPECIAL) would be a way to go, but I think this more confusing.

@@ -378,7 +379,7 @@ private static SourceFile fromZipEntry(String zipURL, Charset inputCharset) {
public static SourceFile fromZipEntry(
String originalZipPath, String absoluteZipPath, String entryPath, Charset inputCharset)
throws MalformedURLException {
String zipEntryPath = JAR_URL_PREFIX + absoluteZipPath + BANG_SLASH + entryPath;
String zipEntryPath = JAR_URL_PREFIX + absoluteZipPath + BANG_SLASH + entryPath.replace(File.pathSeparatorChar, '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be File.separatorChar instead of File.pathSeparatorChar?

My understanding is that separatorChar is the path component separator (/ on Unix, \ on Windows), while pathSeparatorChar is the PATH environment variable separator (: on Unix, ; on Windows).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was fixed with the second commit.

@tjgq tjgq self-assigned this Jan 23, 2019
Copy link
Contributor

@tjgq tjgq left a comment

Choose a reason for hiding this comment

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

Once you address the review comments, can you please squash the two commits into a single one?

@tjgq
Copy link
Contributor

tjgq commented Jan 23, 2019

Created Google internal issue b/123291586

@tjgq tjgq added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Jan 23, 2019
…ogle#3206

And fixes parts of "Support the workflow under Windows. google#9"
(google/j2cl#9) from j2cl
@lgemeinhardt
Copy link
Contributor Author

Once you address the review comments, can you please squash the two commits into a single one?

Squash is done!

@tjgq tjgq closed this in 4ac9af9 Jan 25, 2019
dtsengchromium pushed a commit to dtsengchromium/closure-compiler that referenced this pull request Jan 28, 2019
See also google/j2cl#9.

Fixes google#3206.
Closes google#3207.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=230635801
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes internal-issue-created An internal Google issue has been created to track this GitHub issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants