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

[XBD] Manually Extract proguard configs from .aar files #166

Merged
merged 13 commits into from Jun 29, 2017

Conversation

Projects
None yet
5 participants
@Redth
Copy link
Member

Redth commented Jun 8, 2017

Currently proguard.txt files were not being picked up from inside of .aar files (really any LibraryProjectZip item) by the proguard build task.

In this patch we are going to manually spot them and extract them to the same directory as we output the assembly with the merged resources to so they can be added to @(ProguardConfiguration) which the proguard build task will then see.

Redth added some commits Jun 8, 2017

[XBD] LoadResource now takes assemblyOutputPath
Needed this path so we can consider adding files relative to it (in this case for extracting proguard.txt files from the aar file resources we are embedding)
[XBD] Extract proguard.txt files manually…
… and add them to the `@(ProguardConfiguration)` item group for Xamarin.Android’s build tasks to include in proguard executions.

History:  `proguard.txt` files were _not_ being picked up from inside of .aar files (really any `LibraryProjectZip` item).

In this patch we are going to manually spot them and extract them to the same directory as we output the assembly with the merged resources to so they can be added to `@(ProguardConfiguration)` which the proguard build task will then see.

@Redth Redth requested a review from bholmes Jun 8, 2017

@bholmes
Copy link
Contributor

bholmes left a comment

I am concerned writing back to the merged asm dir. IIRC this is the packages/lib/target folder. I am wondering what will happen if and when the NuGet client and MSBuild tasks stop adding NuGet dlls as references to the csproj and search the directory for content. Can having an unknown file in this folder cause bugs for future build engine updates? I may just be overthinking it. Otherwise I feel the PR does what it intends to do and LGTM.

@Redth

This comment has been minimized.

Copy link
Member

Redth commented Jun 8, 2017

I wonder if @mrward would have any comment on this...

@mrward

This comment has been minimized.

Copy link
Member

mrward commented Jun 8, 2017

Not sure. Where are these new files being created? Into the packages directory itself? So in a PackageReference world these new files would be generated into the ~/.nuget/packages/ directory for the corresponding NuGet package?

@Redth

This comment has been minimized.

Copy link
Member

Redth commented Jun 8, 2017

They would end up going into the local sln dir's ./packages/Xamarin.GooglePlayServices.Basement/lib/MonoAndroid70/ for example, and the file would be proguard.txt

We have a bit of history around the process here, but essentially we modify the .dll files in that same path (in place) to work around thrashing of References in the .csproj causing issues (like extra designer build invocations). This part seems to work well.

We're just talking in this case about adding a .txt to that libs folder.

@mrward

This comment has been minimized.

Copy link
Member

mrward commented Jun 8, 2017

In a PackageReference world there is no $(SolutionDir)/packages so these files I guess would be extracted to ~/.nuget/packages/Xamarin.GooglePlayServices.Basement/lib/MonoAndroid70 and would be available to all projects that use that NuGet package. If these are just .txt and .cfg files then this should be fine.

I would be tempted to just create an Android project, and add a PackageReference, and try a restore and a build to see what happens. Just to test things.

<ItemGroup>
   <PackageReference Include="Xamarin.GooglePlayServices.Basement" Version="1.2.3.4" />
</ItemGroup>

Then from the command line try msbuild /t:restore YourSoluton.sln and an msbuild /t:Build YourSolution.sln and see if that works. The restore will generate a few files in the obj/ directory. The project.assets.json has the assemblies that will be referenced, which I guess will not be a problem here.

Looking at the changes I cannot see any problems. Just adding a ProguardConfiguration task item for the extracted file looks fine to me. I guess adding your own proguard config file to customize things still works here.

Redth added some commits Jun 9, 2017

[XBD] Fix extracting Proguard configs
Moving extracted proguard config files to the intermediate output path (inside the `XbdMerge\proguard` folder).  Also checking each execution for proguard files to include as before they were only included the first build when we extracted them.

// Find any proguard config files we need to emit that were extracted
AarProguardConfiguration = Directory.GetFiles (proguardIntermediateOutputPath)
?.Select (pf => new TaskItem(pf))?.ToArray ()

This comment has been minimized.

@bholmes

bholmes Jun 14, 2017

Contributor

Add Where for extension


string proguardIntermediateOutputPath = null;

public override bool Execute ()

This comment has been minimized.

@bholmes

bholmes Jun 14, 2017

Contributor

Check switching from Debug to Release. Wondering if the LoadResource call will be made as the modified dll is already written. The base may skip in this case.

This comment has been minimized.

@Redth

Redth Jun 15, 2017

Member

So I think this is actually the case, the base will skip this. I think I'm going to need to move this bit into its own task that is always run.

return res;
}

protected override Stream LoadResource (string resourceFullPath, string assemblyName, string assemblyOutputPath)

This comment has been minimized.

@bholmes

bholmes Jun 14, 2017

Contributor

assemblyOutputPath is no longer being used. consider reverting it.

Redth added some commits Jun 15, 2017

[XBD] Moved proguard restore into its own task
Previously tried shoe-horning the extraction and inclusion of proguard config files into the .aar restore task.  However, when we overwrite the source .dll assembly in the restore task, it assumes all the work is already done if the .dll has the resource already embeded into it, so it will skip the rest of the task.  So if we clean our obj/proguard/ directory, the proguard configs would never be re-extracted and never be included again in future builds.

Moving this to a separate task does incur an additional file read of the .aar file, but that step is cached with its own .stamp file so it’s only incurred on the first build.
[XBD] Clean up aar restore task
Leftovers from moving the proguard extraction into its own task that needed cleaning up.
[XBD] Clean up unused parameter
This parameter was added previously but changes were reverted so it’s no longer necessary.
@Redth

This comment has been minimized.

Copy link
Member

Redth commented Jun 26, 2017

Reworked this entire PR and pulled out the proguard config extraction into its own task. I was running into a few cases where the task would be skipped too early to extract the proguard configs in all the correct scenarios when trying to do this as part of the existing aar restore task.

The big one was we checked for the assembly resource for the library project zip and if it was already embedded, we'd skip the rest of the task, so the config files wouldn't get extracted again if the user ran a clean, but after the initial .aar restore had happened and directly overwrote the packages/*.dll files.

I think this also makes the code much easier to remove once this is fixed upstream in Xamarin.Android's core tasks.

Needs a re-review...

@Redth Redth requested a review from mattleibow Jun 26, 2017

@mattleibow
Copy link
Contributor

mattleibow left a comment

@Redth this looks good. I have mostly nitpicking questions, except for maybe the AdditionalFileWrites one.


// Skip entries which are not proguard configs
if (!entry.Name.Equals ("proguard.txt", StringComparison.OrdinalIgnoreCase)
&& !entry.Name.Equals ("proguard.cfg", StringComparison.OrdinalIgnoreCase))

This comment has been minimized.

@mattleibow

mattleibow Jun 26, 2017

Contributor

Just a question/note: are these the only names that are used by Google? Is this name arbitrary, or does the .aar have a specific convention and thus we can assume this?

This comment has been minimized.

@Redth

Redth Jun 27, 2017

Member

Yeah so from what i've seen proguard.txt is the convention. proguard.cfg is something we use so i've thrown that in there on the assumption it may occur in .aar's as well, but i sort of doubt it. In any case i don't think there's harm in adding proguard.cfg too since the chances of some other random file having that name with some other meaning are next to none.

continue;

// Figure out our destination filename
var proguardSaveFilename = Path.Combine (proguardIntermediateOutputPath, saveNameHash + entryCount + ".txt");

This comment has been minimized.

@mattleibow

mattleibow Jun 26, 2017

Contributor

Should we be adding this path to the AdditionalFileWrites?

This comment has been minimized.

@Redth

Redth Jun 27, 2017

Member

Yes. It does get cleaned out regardless with the custom clean task, but it would be good practice to add these, will change.

if (restoreMap == null)
return false;

IAssemblyResolver resolver = CreateAssemblyResolver ();

This comment has been minimized.

@mattleibow

mattleibow Jun 26, 2017

Contributor

Are we using this anywhere? This looks like it may be hitting the file system?

This comment has been minimized.

@Redth

Redth Jun 27, 2017

Member

👍 forgot to remove.

}
}

entryCount++;

This comment has been minimized.

@mattleibow

mattleibow Jun 26, 2017

Contributor

I may be nitpicking, but de we need to increment here? we are basically going through a collection of collections, and if we are incrementing each time in the inner loop, this technically will skip a number. I am just thinking that if we don't have to increment, then don't - we may be debugging this later and we may think that something is wrong when our files go 1, 2, 4, 5, 7, 8 (or something like that).

This comment has been minimized.

@Redth

Redth Jun 27, 2017

Member

Yep, you're right. I was guarding incorrectly against multiple .aar files but hadn't quite fully contemplated the logic :)

@Redth
Copy link
Member

Redth left a comment

Fixed proposed changes.

@Redth Redth merged commit 98a5e50 into master Jun 29, 2017

@Redth Redth deleted the XBD-AarProguardConfig branch Jun 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment