-
Notifications
You must be signed in to change notification settings - Fork 147
Support the workflow under Windows. #9 #26
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
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
1 similar comment
I signed it! |
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
@rluble could you pull this in? |
@@ -84,7 +84,7 @@ public void generateOutputs(List<CompilationUnit> j2clCompilationUnits) { | |||
new JavaScriptImplGenerator(problems, declareLegacyNamespace, type); | |||
|
|||
// If the java type contains any native methods, search for matching native file. | |||
String typeRelativePath = getRelativePath(type); | |||
String typeRelativePath = getRelativePath(type).replace('\\', '/'); |
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.
Could you instead of replacing here change the implementation of getRelativePath and getAbsolutePath to always use "/" instead of File.separator?
AFAIK that should work well even under window, could you see if that works.
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.
Your proposal works for windows. I could only test the getRelativePath case (and not the getAbsolutePath one). I reworked the commit and force-pushed it...
Fix to '/' for all operating system (including windows) in getRelativePath and getAbsolutePath, because the lookup map is using '/' as well
I found and fixed the first file/path issue, while running j2cl native with Windows 10 ... added the following fix ".replace('\', '/')" here "com.google.j2cl.generator.OutputGeneratorStage#generateOutputs:87"
String typeRelativePath = getRelativePath(type).replace('\', '/');
Not sure if this fix is well placed, but is corrects the issue (looking up an windows path in a map with unix paths)...