Skip to content

Conversation

@dellis1972
Copy link
Contributor

@dellis1972 dellis1972 commented Jul 24, 2017

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=58169

We have a bit of a problem with the way we deal with embedding
resources (i.e AndroidResource) into an Android dll.
The process usually goes as follows

  1. Collect all the resources into an intermediate directory
    %(IntermediateOutputPath)__libray_project_imports__
  2. Create a zip file which contains all of those files
  3. Embed that zip into the assembly when we compile.

This seems logical until you look at it from a repeat build
point of view. The senario is that a user changes some C# code
in the android dll. The build process currently does steps
1,2 for ALL changes. So we create a new zip and add that to
the dll. The result of which is as far as the application build
process is concerned its a new zip.. with new files. So we end up
running _AddLibraryProjectsEmbeddedResourceToProject and then
in turn _UpdateAndroidResgen which then in turn will probably
result in an apk build!

All of that happens even though we NEVER touched the resources, we only
touched the C# code.

So in order to fix this we need to look at things slightly differently.
Instead of creating a new zip each time we build, we should incrementally
update/refresh the one we have in the intermediate directory already.
This means not only just updating files but also REMOVING deleted files
from the existing zip file.

By doing this we can maintain the ModifiedDate values in the zip, which
can be used later in the build process to decide if thing were changed or
not.

So to implement this we need to change a few things. Firstly the task
RemoveUnknownFiles has been updated to output a new RemovedFiles
property. This is a ITaskItem which contains a list of the files that
were removed from the build. Next up CreateManagedLibraryResourceArchive
is modified to make use of this new data via the RemovedAndroidResourceFiles
property. We use this to delete files from the intermediate directory and
zip file which have been removed from the project.

We also update CreateManagedLibraryResourceArchive to make use of the
existing zip rather than creating a new one each time. As well as updating
the Files.ExtractAll to allow it to refresh files and remove item from the
zip which we do not expect.

The result of these changes means that changing C# code will result in the
existing zip from the previous build being used for resources. If we do
change a resource then only that item in the zip is refreshed.

These changes then filter down to the ResolveLibraryProjectImports task
which is responsible for maintaining the __library_projects__ directory.
We update this task to make use of the new information available to us in
the zip file to ensure that we only update files which have been changed.
We also only update the .stamp file if we have changed files.

The last piece of the puzzle is to change the _UpdateAndroidResgen target
to NOT use the @(ReferencePath);@(ReferenceDependencyPaths) ItemGroups
as its inputs. If we leave this as is even if we update the C# code the target
will still run because the dll is newer than the $(_AndroidResgenFlagFile).
So we alter this to use the various stamp files we produce to show the
resources were updated. This means that this task will run only when we have
a change.

A new unit test has been added to test out this entire process and to ensure that
the targets and things that should be updated and removed are.

@dellis1972 dellis1972 added the do-not-merge PR should not be merged. label Jul 24, 2017
@dellis1972 dellis1972 changed the title ff [WIP _UpdateAndroidResgen constantly being called Jul 26, 2017
@dellis1972 dellis1972 removed the do-not-merge PR should not be merged. label Jul 26, 2017
@jonpryor
Copy link
Contributor

I suspect that merging PR #700 caused the merge conflicts here. :-(

@dellis1972
Copy link
Contributor Author

@jonpryor I knew that would happen :)

…alled

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=58169

We have a bit of a problem with the way we deal with embedding
resources (i.e AndroidResource) into an Android dll.
The process usually goes as follows

1) Collect all the resources into an intermediate directory
	`%(IntermediateOutputPath)__libray_project_imports__`
2) Create a zip file which contains all of those files
3) Embed that zip into the assembly when we compile.

This seems logical until you look at it from a repeat build
point of view. The senario is that a user changes some C# code
in the android dll. The build process currently does steps
1,2 for ALL changes. So we create a new zip and add that to
the dll. The result of which is as far as the application build
process is concerned its a new zip.. with new files. So we end up
running `_AddLibraryProjectsEmbeddedResourceToProject` and then
in turn `_UpdateAndroidResgen` which then in turn will probably
result in an apk build!

All of that happens even though we NEVER touched the resources, we only
touched the C# code.

So in order to fix this we need to look at things slightly differently.
Instead of creating a new zip each time we build, we should incrementally
update/refresh the one we have in the intermediate directory already.
This means not only just updating files but also REMOVING deleted files
from the existing zip file.

By doing this we can maintain the `ModifiedDate` values in the zip, which
can be used later in the build process to decide if thing were changed or
not.

So to implement this we need to change a few things. Firstly the task
`RemoveUnknownFiles` has been updated to output a new `RemovedFiles`
property. This is a ITaskItem which contains a list of the files that
were removed from the build. Next up `CreateManagedLibraryResourceArchive`
is modified to make use of this new data via the `RemovedAndroidResourceFiles`
property. We use this to delete files from the intermediate directory and
zip file which have been removed from the project.

We also update `CreateManagedLibraryResourceArchive` to make use of the
existing zip rather than creating a new one each time. As well as updating
the `Files.ExtractAll` to allow it to refresh files and remove item from the
zip which we do not expect.

The result of these changes means that changing C# code will result in the
existing zip from the previous build being used for resources. If we do
change a resource then only that item in the zip is refreshed.

These changes then filter down to the `ResolveLibraryProjectImports` task
which is responsible for maintaining the `__library_projects__` directory.
We update this task to make use of the new information available to us in
the zip file to ensure that we only update files which have been changed.
We also only update the .stamp file if we have changed files.

The last piece of the puzzle is to change the `_UpdateAndroidResgen` target
to NOT use the `@(ReferencePath);@(ReferenceDependencyPaths)` ItemGroups
as its inputs. If we leave this as is even if we update the C# code the target
will still run because the dll is newer than the `$(_AndroidResgenFlagFile)`.
So we alter this to use the various stamp files we produce to show the
resources were updated. This means that this task will run only when we have
a change.

A new unit test has been added to test out this entire process and to ensure that
the targets and things that should be updated and removed are.
@dellis1972
Copy link
Contributor Author

build

1 similar comment
@dellis1972
Copy link
Contributor Author

build

@jonpryor jonpryor merged commit 8688832 into dotnet:master Jul 31, 2017
dellis1972 added a commit that referenced this pull request Aug 1, 2017
…alled (#702)

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=58169

We have a bit of a problem with the way we deal with embedding
resources (i.e AndroidResource) into an Android dll.
The process usually goes as follows

1) Collect all the resources into an intermediate directory
	`%(IntermediateOutputPath)__libray_project_imports__`
2) Create a zip file which contains all of those files
3) Embed that zip into the assembly when we compile.

This seems logical until you look at it from a repeat build
point of view. The senario is that a user changes some C# code
in the android dll. The build process currently does steps
1,2 for ALL changes. So we create a new zip and add that to
the dll. The result of which is as far as the application build
process is concerned its a new zip.. with new files. So we end up
running `_AddLibraryProjectsEmbeddedResourceToProject` and then
in turn `_UpdateAndroidResgen` which then in turn will probably
result in an apk build!

All of that happens even though we NEVER touched the resources, we only
touched the C# code.

So in order to fix this we need to look at things slightly differently.
Instead of creating a new zip each time we build, we should incrementally
update/refresh the one we have in the intermediate directory already.
This means not only just updating files but also REMOVING deleted files
from the existing zip file.

By doing this we can maintain the `ModifiedDate` values in the zip, which
can be used later in the build process to decide if thing were changed or
not.

So to implement this we need to change a few things. Firstly the task
`RemoveUnknownFiles` has been updated to output a new `RemovedFiles`
property. This is a ITaskItem which contains a list of the files that
were removed from the build. Next up `CreateManagedLibraryResourceArchive`
is modified to make use of this new data via the `RemovedAndroidResourceFiles`
property. We use this to delete files from the intermediate directory and
zip file which have been removed from the project.

We also update `CreateManagedLibraryResourceArchive` to make use of the
existing zip rather than creating a new one each time. As well as updating
the `Files.ExtractAll` to allow it to refresh files and remove item from the
zip which we do not expect.

The result of these changes means that changing C# code will result in the
existing zip from the previous build being used for resources. If we do
change a resource then only that item in the zip is refreshed.

These changes then filter down to the `ResolveLibraryProjectImports` task
which is responsible for maintaining the `__library_projects__` directory.
We update this task to make use of the new information available to us in
the zip file to ensure that we only update files which have been changed.
We also only update the .stamp file if we have changed files.

The last piece of the puzzle is to change the `_UpdateAndroidResgen` target
to NOT use the `@(ReferencePath);@(ReferenceDependencyPaths)` ItemGroups
as its inputs. If we leave this as is even if we update the C# code the target
will still run because the dll is newer than the `$(_AndroidResgenFlagFile)`.
So we alter this to use the various stamp files we produce to show the
resources were updated. This means that this task will run only when we have
a change.

A new unit test has been added to test out this entire process and to ensure that
the targets and things that should be updated and removed are.
jonpryor pushed a commit that referenced this pull request Sep 2, 2020
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1139590
Context: dotnet/java-interop@afbc5b3

Changes: dotnet/java-interop@dcaf794...afbc5b3

  * dotnet/java-interop@afbc5b32: [C++] Get rid of compiler warnings (#703)
  * dotnet/java-interop@e295b498: [generator] Fix localization error. (#702)
  * dotnet/java-interop@5a0e37e0: [generator] Support 'managedOverride' metadata (#701)
  * dotnet/java-interop@f6c12ba3: [Xamarin.Android.Tools.Bytecode] Hide Kotlin synthetic constructors (#700)
  * dotnet/java-interop@0334f424: [generator] Hide Java.Lang.Object infrastructure (#698)
  * dotnet/java-interop@c0fcc435: [jcw-gen] Make MSBuild warning/error strings localizable. (#695)

Fixes a handful of warnings reported by GCC and clang when building
for different platforms.

`-Wshadow` warnings:

	monodroid-glue.cc:1609:8: warning: declaration shadows a static data member of 'xamarin::android::internal::MonodroidRuntime' [-Wshadow]
	void *api_dso_handle = nullptr;
	              ^
	monodroid-glue.cc:102:25: note: previous declaration is here
	void *MonodroidRuntime::api_dso_handle = nullptr;

Fix by renaming the local variable.  No functionality changes result
from this fix.

MinGW builds report:

	xa-internal-api.cc: In member function ‘virtual mono_bool xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_up_state(const char*, mono_bool*)’:
	xa-internal-api.cc:24:86: warning: unused parameter ‘ifname’ [-Wunused-parameter]
	   24 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_up_state (const char *ifname, mono_bool *is_up)
	      |                                                                          ~~~~~~~~~~~~^~~~~~
	xa-internal-api.cc:24:105: warning: unused parameter ‘is_up’ [-Wunused-parameter]
	   24 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_up_state (const char *ifname, mono_bool *is_up)
	      |                                                                                              ~~~~~~~~~~~^~~~~
	xa-internal-api.cc: In member function ‘virtual mono_bool xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_supports_multicast(const char*, mono_bool*)’:
	xa-internal-api.cc:34:96: warning: unused parameter ‘ifname’ [-Wunused-parameter]
	   34 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_supports_multicast (const char *ifname, mono_bool *supports_multicast)
	      |                                                                                    ~~~~~~~~~~~~^~~~~~
	xa-internal-api.cc:34:115: warning: unused parameter ‘supports_multicast’ [-Wunused-parameter]
	   34 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_supports_multicast (const char *ifname, mono_bool *supports_multicast)
	      |                                                                                                        ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
	xa-internal-api.cc: In member function ‘virtual int xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_get_dns_servers(void**)’:
	xa-internal-api.cc:44:66: warning: unused parameter ‘dns_servers_array’ [-Wunused-parameter]
	   44 | MonoAndroidInternalCalls_Impl::monodroid_get_dns_servers (void **dns_servers_array)
	      |                                                           ~~~~~~~^~~~~~~~~~~~~~~~~
	xa-internal-api.cc: In member function ‘virtual int xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_getifaddrs(_monodroid_ifaddrs**)’:
	xa-internal-api.cc:54:82: warning: unused parameter ‘ifap’ [-Wunused-parameter]
	   54 | MonoAndroidInternalCalls_Impl::monodroid_getifaddrs (struct _monodroid_ifaddrs **ifap)
	      |                                                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
	xa-internal-api.cc: In member function ‘virtual void xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_freeifaddrs(_monodroid_ifaddrs*)’:
	xa-internal-api.cc:64:82: warning: unused parameter ‘ifa’ [-Wunused-parameter]
	   64 | MonoAndroidInternalCalls_Impl::monodroid_freeifaddrs (struct _monodroid_ifaddrs *ifa)
	      |                                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~

Fix by ignoring the unused parameters only during Windows builds
by using the C++17 attribute `[[maybe_unused]]`

MinGW also reports:

	embedded-assemblies.cc: In static member function ‘static ssize_t xamarin::android::internal::EmbeddedAssemblies::do_read(int, void*, size_t)’:
	embedded-assemblies.cc:663:26: warning: conversion from ‘size_t’ {aka ‘long long unsigned int’} to ‘unsigned int’ may change value [-Wconversion]
	  663 |   ret = ::read (fd, buf, count);
	      |

It is caused by **read**(2) declared in MinGW headers with the third
parameter using an `unsigned int` type instead of the standard
`size_t`.  Fixed by casting properly on Windows builds.
@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