switch to facebook proguard #209
Conversation
atsushieno
force-pushed
the
bundle-proguard
branch
3 times, most recently
from
8d09508
to
e120efd
Compare
|
After we switch to proguard 5.2.1, we should be able to eliminate _AdjustJavacVersionArguments target: |
|
We'll need to get permission to distribute proguard before we can merge this PR. |
atsushieno
force-pushed
the
bundle-proguard
branch
from
e120efd
to
1056e82
Compare
- it is based on proguard 5.2.1 which should be able to process Java8 bytecode. - facebook proguard is reportedly 20%~ faster.
atsushieno
force-pushed
the
bundle-proguard
branch
3 times, most recently
from
eaca0c8
to
45ed27a
Compare
| @@ -0,0 +1,12 @@ | |||
| <Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
|
|
|||
| <PropertyGroup> | |||
There was a problem hiding this comment.
Xamarin Studio is..."finicky"...about project formats.
Please follow https://github.com/xamarin/xamarin-android/blob/6cfe2a3ee8943687fd325c46979f915ba619d274/build-tools/libzip/libzip.mdproj as an example. Specifically, to appease Xamarin Studio, proguard.mdproj must have a <PropertyGroup/> for each "configuration", Debug and Release.
| @@ -2037,6 +2039,7 @@ because xbuild doesn't support framework reference assemblies. | |||
| Condition="'$(AndroidEnableMultiDex)' == 'True' And '$(AndroidCustomMainDexListFile)' == ''" | |||
| ToolPath="$(MainDexClassesToolPath)" | |||
| ToolExe="$(MainDexClassesToolExe)" | |||
| ProguardHome="$(_MonoAndroidToolsDirectory)proguard" | |||
There was a problem hiding this comment.
I think this should be a new "public" MSBuild property, e.g. $(ProguardToolPath), so that alternate versions can be overridden.
There was a problem hiding this comment.
$(ProguardToolPath) already existed and I avoided possible different use of the variable. Though as long as I reviewed it seems to be alright to use it.
|
I've started the license review for including proguard. We'll see how that turns out. |
atsushieno
force-pushed
the
bundle-proguard
branch
from
45ed27a
to
ad28fde
Compare
multidex `mainDexClasses` tool depends on proguard, and its premise on
proguard directory structure is that it has these files under $PROGUARD_HOME:
- lib/proguard.jar
- bin/proguard.sh (or .bat)
If we make it so, we could keep using the official `mainDexClasses`
with no changes and hence no need to maintain that moving target.
The change subsequently required changes to `ProguardJarPath`.
Also `ProguardToolPath` needs to change (it should have been done in the
previous fix).
Changing proguard toolchain paths should have also done for our custom
configuration support. Custom proguard build does not come up with
proguard-android*.txt config files from Android SDK tools. We still use
those custom configs from Android SDK tools, so instead of depending on
$PROGUARD_HOME, we'd depend on `AndroidSdkDirectory` (as `{sdk}` in those
config files implies).
Lastly, CreateMultiDexMainDexClasses task now takes PROGUARD_HOME argument
above to actually set as an environment variable, to take effect.
atsushieno
force-pushed
the
bundle-proguard
branch
from
ad28fde
to
35c02b3
Compare
|
Review has completed. Proguard is GPLv2, which has a number of distribution requirements that we'd really rather not deal with (e.g. providing source on request). What we can do and are looking into doing is updating the macOS and Windows installers to install proguard as part of their operation. So long as we download prebuilt binaries -- which e.g. maven.org provides -- we can let the site we're downloading worry about providing source code, so long as we also provide a "notice" in the installer that proguard is being used. ...which leaves the question of what to do about this PR. We can't accept it as-is; for starters we don't want the proguard external submodule. We may want to consider adding additional "proguard version checks" to our tasks, and if the proguard version is too old -- v4.x -- then either issue an error or disable the feature which requires a newer proguard. We might also want to document how to "side-load" proguard and update project files to use the "side-loaded" proguard. |
|
That sounds like someone else who work on that kind of stuff should become responsible to support proguard, not me. |
|
It is being superceded by #267 . |
Context: https://github.com/xamarin/java.interop/blob/5cf61f5/Makefile#L149 When we ported the NUnit tests from `Makefile` to `MSBuild`, there was a target missing that copies `libjava-interop.dylib` from `bin/Debug` to `bin/TestDebug`. It went unnoticed on Windows, because these tests are not passing yet there.
Note that this will result in distributing proguard.jar within our product that you'd like to discuss whether to go or not.