Skip to content

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Nov 27, 2017

Context: https://bugzilla.xamarin.com/show_bug.cgi?id=56834

A default multi-dex application will print a warning to the console
such as:

Warning: can't write resource [META-INF/MANIFEST.MF] (Duplicate zip entry [android-support-multidex.jar:META-INF/MANIFEST.MF])

This is due to most jar files containing a META-INF/MANIFEST.MF,
which really just needs to be excluded from the proguard command.
We can do this by adding a filter to the end of each jar file passed
via the -injars command line option.

Changes:

  • Added a $(_AndroidProguardInputJarFilter) internal property, which
    defaults to (!META-INF/MANIFEST.MF) so we ignore the most common warning
    by default
  • Added a ProguardInputJarFilter property to the <Proguard /> and
    <CreateMultiDexMainDexClassList /> tasks
  • Added assertions to multi-dex unit tests making sure we aren't getting
    the warning anymore
  • Minor fix to clear the ghprbPullLongDescription environment variable during
    tests, otherwise my PR/commit message causes my new test assertions to fail

@dnfclas
Copy link

dnfclas commented Nov 27, 2017

@jonathanpeppers,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@jonpryor
Copy link
Contributor

Should this be a public MSBuild property at all? I'm not sure that this is an extension point that we want to support, or -- alternatively -- I don't fully foresee what we may be signing ourselves up for by exposing this as a public property.

Additionally, I'm not sure about the longevity of ProGuard, given that Google is working on d8/etc. as a replacement.

I'd feel better if this were a private -- _-prefixed -- property, or hardcoded in the task.

@jonathanpeppers jonathanpeppers changed the title [Xamarin.Android.Build.Tasks] added $(AndroidProguardExclude) [Xamarin.Android.Build.Tasks] added filter to proguard commands Nov 27, 2017
@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Nov 28, 2017

🤦‍♂️ my two new tests are failing because of this PR's description being passed as an environment variable ghprbPullLongDescription:

ghprbPullLongDescription = Context: https://bugzilla.xamarin.com/show_bug.cgi?id=56834\r\n\r\nA default multi-dex application will print a warning to the console\r\nsuch as:\r\n```\r\nWarning: can't write resource [META-INF/MANIFEST.MF] (Duplicate zip entry [android-support-multidex.jar:META-INF/MANIFEST.MF])\r\n```\r\n\r\nThis is due to most jar files containing a `META-INF/MANIFEST.MF`,\r\nwhich really just needs to be excluded from the proguard command.\r\nWe can do this by adding a filter to the end of each jar file passed\r\nvia the `-injars` command line option.\r\n\r\nChanges:\r\n- Added a `$(_AndroidProguardInputJarFilter)` internal property, which\r\ndefaults to `(!META-INF/MANIFEST.MF)` so we ignore the most common warning\r\nby default\r\n- Added a `ProguardInputJarFilter` property to the `<Proguard />` and\r\n`<CreateMultiDexMainDexClassList />` tasks\r\n- Added assertions to multi-dex unit tests making sure we aren't getting\r\nthe warning anymore

I put Warning: can't write resource [META-INF/MANIFEST.MF] (Duplicate zip entry) in the PR description (as it probably needs to be).

I think the easiest thing is to clear out ghprbPullLongDescription when Xamarin.Android.Build.Tests runs xbuild/msbuild, it is not needed there anyway...

Context: https://bugzilla.xamarin.com/show_bug.cgi?id=56834

A default multi-dex application will print a warning to the console
such as:
```
Warning: can't write resource [META-INF/MANIFEST.MF] (Duplicate zip entry [android-support-multidex.jar:META-INF/MANIFEST.MF])
```

This is due to most jar files containing a `META-INF/MANIFEST.MF`,
which really just needs to be excluded from the proguard command.
We can do this by adding a filter to the end of each jar file passed
via the `-injars` command line option.

Changes:
- Added a `$(_AndroidProguardInputJarFilter)` internal property, which
defaults to `(!META-INF/MANIFEST.MF)` so we ignore the most common warning
by default
- Added a `ProguardInputJarFilter` property to the `<Proguard />` and
`<CreateMultiDexMainDexClassList />` tasks
- Added assertions to multi-dex unit tests making sure we aren't getting
the warning anymore
- Minor fix to clear the `ghprbPullLongDescription` environment variable during
tests, otherwise my PR/commit message causes my new test assertions to fail
@jonpryor jonpryor merged commit f5ecb28 into dotnet:master Nov 28, 2017
@jonathanpeppers jonathanpeppers deleted the proguard-excludes branch November 28, 2017 20:36
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 4, 2017
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.

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 added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 4, 2017
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 added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 4, 2017
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 added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 6, 2017
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 added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 6, 2017
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.
jonpryor pushed a commit that referenced this pull request Dec 7, 2017
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 #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 `'` elsewhere. This also resolved the issues in the
`Desugar` test on Windows, for which I also added an additional
assertion.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants