[Xamarin.Android.Build.Tasks] fix for proguard enclosing char on windows #1068
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jonathanpeppers, |
jonathanpeppers
changed the title
jonathanpeppers
force-pushed
the
proguard-windows-enclosingchar
branch
from
c847141 to
ac0039b
Compare
Contributor
|
build |
Contributor
|
@jonathanpeppers we have to be really careful with this. The quoting is weird between windows and osx, especially with paths with spaces. that said this does look reasonable. |
dellis1972
approved these changes
Dec 6, 2017
Member
Author
|
Yeah, the problem is #1049 can definitely break on Windows. @dellis1972 you think I should add a |
Contributor
jonathanpeppers
force-pushed
the
proguard-windows-enclosingchar
branch
from
ac0039b to
9b5cfb0
Compare
I noticed on Windows, the `Desugar(True, True, True)` test was failing with: ``` java.io.IOException: Can't read [E:\A\_work\1\s\bin\TestDebug\temp\Desugar(True,True,True)\obj\Release\android\bin\classes\classes.zip(!META-INF\MANIFEST.MF)] (No such file or directory) ``` However, running locally proved the file does exist. Somehow dotnet#1049 introduced a case where proguard is failing due to the combination the new `(!META-INF/MANIFEST.MF)` filter and the parameters passed to proguard in this test. Looking into it, we had an odd form of how the paths were being passed on Windows using a combination of single and double-quotes: ``` -injars "'path1(!META-INF/MANIFEST.MF)';'path2(!META-INF/MANIFEST.MF)'" ``` The way these paths were passed was not working on Windows after adding this new filter--but only for this `Desugar` test... Most cases were working, such as our tests around multi-dex and proguard. It worked correctly on Windows when specified as: ``` -injars "path1(!META-INF/MANIFEST.MF)";"path2(!META-INF/MANIFEST.MF)" ``` So keep things consistent, I refactored the code to use " on Windows and ' on other platforms. This also resolved the issues in the `Desugar` test on Windows, for which I also added an additional assertion.
jonathanpeppers
force-pushed
the
proguard-windows-enclosingchar
branch
from
9b5cfb0 to
431316a
Compare
jonathanpeppers
added a commit
to jonathanpeppers/xamarin-android
that referenced
this pull request
Dec 21, 2017
Fixes: dotnet#1135 Context: dotnet#1068 When making changes in dotnet#1068, I added a unit test that builds an application with both spaces and parentheses in its path. However, this was not enough to test everything; the test also needed an `AndroidJavaLibrary` build item with spaces and parentheses in its name! Changes: - Made a `Hello (World).jar` file, that has a single class in it - Added the jar file as a `AndroidJavaLibrary`, in the `BuildApplicationWithSpacesInPath` test - Reverted `Proguard.cs` and `CreateMultiDexMainDexClassList.cs` from what I did in dotnet#1068 -- Dean tried to warn me - Move the `(!META-INF/MANIFEST.MF)` filter expression outside the single quotes PR dotnet#1068 was a good attempt at trying to cleanup the code, but I could not get the manifest filter to work along with a path including a parentheses.
jonathanpeppers
added a commit
to jonathanpeppers/xamarin-android
that referenced
this pull request
Dec 21, 2017
Fixes: dotnet#1135 Context: dotnet#1068 When making changes in dotnet#1068, I added a unit test that builds an application with both spaces and parentheses in its path. However, this was not enough to test everything; the test also needed an `AndroidJavaLibrary` build item with spaces and parentheses in its name! Changes: - Made a `Hello (World).jar` file, that has a single class in it - Added the jar file as a `AndroidJavaLibrary`, in the `BuildApplicationWithSpacesInPath` test - Reverted `Proguard.cs` and `CreateMultiDexMainDexClassList.cs` from what I did in dotnet#1068 -- Dean tried to warn me - Move the `(!META-INF/MANIFEST.MF)` filter expression outside the single quotes - For the `-injars` expression to work on *both* macOS and Windows, I had to enclose the entire expression in double-quotes PR dotnet#1068 was a good attempt at trying to cleanup the code, but I could not get the manifest filter to work along with a path including a parentheses.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed on Windows, the
Desugar(True, True, True)test was failingwith:
However, running locally proved the file does exist.
Somehow #1049 introduced a case where proguard is failing due to the
combination the new
(!META-INF/MANIFEST.MF)filter and the parameterspassed to proguard in this test.
Looking into it, we had an odd form of how the paths were being passed
on Windows using a combination of single and double-quotes:
The way these paths were passed was not working on Windows after adding
this new filter--but only for this
Desugartest... Most cases wereworking, such as our tests around multi-dex and proguard.
It worked correctly on Windows when specified as:
So keep things consistent, I refactored the code to use " on Windows and
' on other platforms. This also resolved the issues in the
Desugartest onWindows, for which I also added an additional assertion.