manifest-update writes new manifest, but old copy is still used.

[MartinMReed]

If you have in your pom, the manifest-update mojo will correctly update it and write out the new version as . However future mojos appear to use whatever you have in . So if they are both the same, you are fine. However, if you have only set , then it is wrong. You should not be required to set , so I believe this is a bug.

This might be related to how you are trying to switch over and use the updated path:

ManifestUpdateMojo.java#L662:
project.getProperties().setProperty( "android.manifestFile", updatedManifestFile.getAbsolutePath() );

I don't think the other mojos are using the project property value instead of the pom value ().

Running version 4.0.0-rc.2.

[william-ferguson-au]

Which other mojos?
On 17 Nov 2014 17:51, "Martin M Reed" [email protected] wrote:

If you have in your pom, the manifest-update mojo will correctly update it
and write out the new version as . However future mojos appear to use
whatever you have in . So if they are both the same, you are fine. However,
if you have only set , then it is wrong. You should not be required to set
, so I believe this is a bug.

This might be related to how you are trying to switch over and use the
updated path:

ManifestUpdateMojo.java#L662:
project.getProperties().setProperty( "android.manifestFile",
updatedManifestFile.getAbsolutePath() );

I don't think the other mojos are using the project property value instead
of the pom value ().

Running version 4.0.0-rc.2.

—
Reply to this email directly or view it on GitHub
#508.

[MartinMReed]

Sorry, I haven't had time to go through the source and investigate. What I know at this point is that the manifest is updated and moved to the new location, but the final build did not use it. So I'm guessing it was a compile, or dex, or package mojo that needed it and found the wrong one.

[greek1979]

I believe @MartinMReed has got the point, as I personally stumbled upon the very same issue when migrating from to 4.0.0 AMP from an old (3.9.0) version. After taking some time to investigate (and to study the plugin source code), here is the situation and the root cause of all the troubles with updating the manifest.

Suppose one needs to run ManifestUpdateMojo to make some build-time adjustments to application manifest, like setting proper and unique version code, or injecting profile-specific application icon. To keep original manifest safe, we set androidManifestFile to point to its original location, and updatedManifestFile to some arbitrary temporary location, where the resulting manifest will end up. Everything runs just fine, and manifest is properly updated.

Now we also need to produce an apk artifact, which involves taking that updated manifest and, say, renaming application package, or creating R file in a custom package. Something that is performed by GenerateSourcesMojo, which runs as part of a modified build lifecycle. Same applies to ApkMojo and other lifecycle default participants. These grab the manifest from androidManifestFile location, completely ignoring new location! ManifestUpdateMojo does update the parameter to reflect new location, but it is done in the context of standalone mojo invocation, hence all runtime parameter changes are lost by the time other mojo-s are executed!

[greek1979]

The simplest solution to the above problem would be to split plugin configuration into major, "default" section, with androidManifestFile pointing to updated (new) manifest file location, and dedicated configuration for "manifest-update" goal with both androidManifestFile and updatedManifestFile present. Not exactly straightforward, but it works.

BUT it doesn't work at all if original (source) manifest is still located in project root, as it is the case with most legacy Android projects, I believe! The reason for this is "non-standard layout check" introduced in AMP 4.0.0, which looks for the manifest in pre-4 location, finds it, then examines androidManifestFile parameter to see if it indeed points to that "old" location.. And since it does not (see my previous comment), it spits out a warning and fails the build by default.

There is an option to employ sourceManifestFile parameter, but it is actually referenced in copyManifest() method, which is invoked AFTER checking for non-standard layout; if the build was failed, manifest copying logic is ignored. I will try to submit a PR to extend manifest mangling logic.

[Shusshu]

I think there is definetely some weird stuff with these 3 properties (sourceManifestFile, androidManifestFile, updatedManifestFile)

I'm currently implementing the manifest-merger v2 from google:
See issue #519
See branch https://github.com/simpligility/android-maven-plugin/tree/feature-manifest-merger-v2

As you can see in that issue (and also in the branch)
I use the following config:

<sourceManifestFile>${project.basedir}/src/main/AndroidManifest.xml</sourceManifestFile>
<androidManifestFile>${project.build.directory}/AndroidManifest.xml</androidManifestFile>

and the default value of updatedManifestFile has changed to

 ${project.build.directory}/AndroidManifest.xml 

I think we could achieve the same with 2 properties but we'd have to be sure to be backwards compatible

[greek1979]

Here lies the problem. If updatedManifestFile points to a build-folder by default, it essentially acts as manifest copying, thereby duplicating existing copyManifest() logic, but in the scope of a standalone mojo (ManifestUpdateMojo). Doesn't make too much sense to me, because after copying the manifest, it has to be picked up by other lifecycle mojos, if they are run as part of build process, of course.

Which implies that a user has to explicitly set androidManifestFile (in configuration shared by all plugin mojos) to a location in build folder; after that the same configuration block cannot be used by ManifestUpdateMojo anymore...

[Shusshu]

after that the same configuration block cannot be used by ManifestUpdateMojo anymore...

What do you mean by that?

For the ManifestMergerMojo it makes sense to point the androidManifestFile & updatedManifestFile to the build folder.
The mojo will take the source, process it and put it in the build folder.
Other mojo will use the androidManifestFile path (which is in the build folder) and all is well ;)
Edit: Actually for this mojo you would only need 2 properties: sourceManifestFile & androidManifestFile

On the other hand for the ManifestUpdateMojo it does not always makes sense to place the manifest in the build folder for example if you use the autoIncrement flag you want to keep (and probably commit to git) the new manifest file.

[greek1979]

The problem is about the sequence (yes, the default one, I admit) of running all the mojos. According to a classic APK or AAR build lifecycle, GenerateSourcesMojo runs first, and does the manifest file copying (see sourceManifestFile and who uses it). Then, ManifestUpdateMojo is given a chance to execute, hence both "source" and "updated" manifest files should point to a new location. And finally, all other mojos are started. At the same time it is very common to share plugin execution configuration amongst all "default" mojos, including GenerateSourcesMojo and, say, ApkMojo. Therefore one has to keep ManifestUpdateMojo configuration separately, as androidManifestFile parameter is expected to have different value for different mojos.

Moreover, given current configuration setup and default values of parameters, one would also have to explicitly specify updatedManifestFile and point it to the same file, as otherwise ManifestUpdateMojo would simply copy the manifest to its default location (src/main).

See #533 for suggested minimum changes. These should not break new ManifestMerger v2, I believe

[malachid]

"hence both "source" and "updated" manifest files should point to a new
location"

I think that's the miscommunication. The idea behind the
sourceManifestFile is that it will not be altered. It should point to the
one under source control. Ideally, doing a build should not require a
commit.

I'm currently doing:
sourceManifestFile = ${project.basedir}/src/main/android/AndroidManifest.xml
androidManifestFile = ${project.build.directory}/AndroidManifest.xml
failOnNonStandardStructure = false

then on the manifest update:
versionName = ${project.version}
versionCodeUpdateFromVersion = true

As such, the src/main/android/AndroidManifest.xml is never altered by a
build; but the generated target/AndroidManifest.xml is configured per the
pom's version.

On Wed Dec 10 2014 at 7:28:21 AM Leonid [email protected] wrote:

The problem is about the sequence (yes, the default one, I admit) of
running all the mojos. According to a classic APK or AAR build lifecycle,
GenerateSourcesMojo runs first, and does the manifest file copying (see
sourceManifestFile and who uses it). Then, ManifestUpdateMojo is given
a chance to execute, hence both "source" and "updated" manifest files
should point to a new location. And finally, all other mojos are started.
At the same time it is very common to share plugin execution configuration
amongst all "default" mojos, including GenerateSourcesMojo and, say,
ApkMojo. Therefore one has to keep ManifestUpdateMojo configuration
separately, as androidManifestFile parameter is expected to have
different value for different mojos.

Moreover, given current configuration setup and default values of
parameters, one would also have to explicitly specify
updatedManifestFile and point it to the same file, as otherwise
ManifestUpdateMojo would simply copy the manifest to its default location
(src/main).

See #533 #533
for suggested minimum changes. These should not break new
ManifestMerger v2, I believe

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[greek1979]

It is interesting though, that judging by ManifestUpdateMojo source, the default value for updatedManifestFile is ${project.basedir}/src/main/AndroidManifest.xml. Hence if you do not explicitly provide any other location, the mojo would update version code, and the save resulting manifest under src/main. Therefore to get all build-time changes to the manifest in one place, you also need to set updatedManifestFile to, say, ${project.build.directory}/AndroidManifest.xml. Three configuration parameters, with two having the same value... A bit too much configuration, imho.

As for setting failOnNonStandardStructure to false, it would be nice to let AMP do all the usual project layout checks. If we have an extra safety option, why not to use it?

[malachid]

The androidManifestFile parameter in AbstractAndroidMojo overrides the
android.manifest property. So in my case, that points to
target/AndroidManifest.xml.

As for the failOnNonStandardStructure... This application has had the
manifest file in src/main/android for over 4 years and I would rather not
move it. Also, we have a symlink at the root for one of the Eclipse users,
which ends up triggering the androidManifestFilePre4.exists() logic, even
though it isn't in use from Maven.

On Wed Dec 10 2014 at 8:04:22 AM Leonid [email protected] wrote:

It is interesting though, that judging by ManifestUpdateMojo source, the
default value for updatedManifestFile is
${project.basedir}/src/main/AndroidManifest.xml. Hence if you do not
explicitly provide any other location, the mojo would update version code,
and the save resulting manifest under src/main. Therefore to get all
build-time changes to the manifest in one place, you also need to set
updatedManifestFile to, say,
${project.build.directory}/AndroidManifest.xml. Three configuration
parameters, with two having the same value... A bit too much configuration,
imho.

As for setting failOnNonStandardStructure to false, it would be nice to
let AMP do all the usual project layout checks. If we have an extra safety
option, why not to use it?

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[greek1979]

Also, we have a symlink at the root for one of the Eclipse users,
which ends up triggering the androidManifestFilePre4.exists() logic, even
though it isn't in use from Maven.

That's exactly the point! If one's application has its android manifest placed in "new default" src/main directory, then this is good; for those apps that still keep manifest in project root for legacy reasons, AMP will keep firing project layout warnings unless developer explicitly turns them off. And if warnings are disabled, he might overlook some other issue with project structure, I guess.

The bottom line of this long discussion is, imho, is that having 3 parameters to configure 2 file locations looks like an overhead to me, as at least two parameters have to be assigned the same value to not to break the project. See my pull request #533 for a way to at least partially overcome this situation.

[malachid]

Based on your comment, I relooked at my poms and found that I was in fact
setting the updatedManifestFile in one of my profiles to point to
${project.build.directory}/AndroidManifest.xml for the update-manifest goal.
I'm also pointing androidManifestFile to
${project.basedir}/src/main/android/AndroidManifest.xml for the undeploy
goal, since I attach it to pre-clean and the target version may not yet
exist.

I agree with you that we need to use some sensible defaults here. We
should be able to specify at most two locations and be done with it.

Looking at the pull request, I have two comments (including in this
conversation rather than in the PR itself):

1. I don't think the validateStandardLocations changes wouldn't have any
   effect on my need to use it, as I in essence have two source manifests. The
   original and the symlink. Maybe we need logic to whitelist rather than
   blacklist. Is the valid location there, rather than is any invalid
   location available. Of course, in my case that still wouldn't help -- but
   it would help others.
2. Removing the default from the updatedManifestFile; but still having it
   specify the same property as another parameter (androidManifestFile) in the
   parent class...

which leads me to thinking... maybe we should change the
AbstractAndroidMojo defaults:

1. change sourceManifestFile to point to
   ${project.basedir}/src/main/AndroidManifest.xml instead of nothing
2. change androidManifestFile to point to
   ${project.build.directory}/AndroidManifest.xml instead of
   ${project.basedir}/src/main/AndroidManifest.xml
   then in ManifestUpdateMojo:
3. remove updatedManifestFile (which points to
   ${project.basedir}/src/main/AndroidManifest.xml) and rely on
   androidManifestFile from the parent class
   then in UndeployMojo:
4. change it to rely on the sourceManifestFile instead of the
   androidManifestFile
   etc etc... there are probably quite a few places to check and/or tweak

Obviously this could have repercussions on quite a few people. It's worth
us figuring out the right approach to take.

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 1:46 AM, Leonid [email protected] wrote:

Also, we have a symlink at the root for one of the Eclipse users,
which ends up triggering the androidManifestFilePre4.exists() logic, even
though it isn't in use from Maven.

That's exactly the point! If one's application has its android manifest
placed in "new default" src/main directory, then this is good; for those
apps that still keep manifest in project root for legacy reasons, AMP will
keep firing project layout warnings unless developer explicitly turns them
off. And if warnings are disabled, he might overlook some other issue with
project structure, I guess.

The bottom line of this long discussion is, imho, is that having 3
parameters to configure 2 file locations looks like an overhead to me, as
at least two parameters have to be assigned the same value to not to break
the project. See my pull request #533
#533 for a way
to at least partially overcome this situation.

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[Shusshu]

Yep I like that approach.

I'd like everyone to answer a question that was asked elsewhere:
After updating the manifest using the manifest-update or manifest-merger
mojo where would you want to have the updated manifest located? In target
or in the src?
On 11 Dec 2014 17:21, "Malachi de AElfweald" [email protected]
wrote:

Based on your comment, I relooked at my poms and found that I was in fact
setting the updatedManifestFile in one of my profiles to point to
${project.build.directory}/AndroidManifest.xml for the update-manifest
goal.
I'm also pointing androidManifestFile to
${project.basedir}/src/main/android/AndroidManifest.xml for the undeploy
goal, since I attach it to pre-clean and the target version may not yet
exist.

I agree with you that we need to use some sensible defaults here. We
should be able to specify at most two locations and be done with it.

Looking at the pull request, I have two comments (including in this
conversation rather than in the PR itself):

1. I don't think the validateStandardLocations changes wouldn't have any
   effect on my need to use it, as I in essence have two source manifests.
   The
   original and the symlink. Maybe we need logic to whitelist rather than
   blacklist. Is the valid location there, rather than is any invalid
   location available. Of course, in my case that still wouldn't help -- but
   it would help others.
2. Removing the default from the updatedManifestFile; but still having it
   specify the same property as another parameter (androidManifestFile) in
   the
   parent class...

which leads me to thinking... maybe we should change the
AbstractAndroidMojo defaults:

1. change sourceManifestFile to point to
   ${project.basedir}/src/main/AndroidManifest.xml instead of nothing
2. change androidManifestFile to point to
   ${project.build.directory}/AndroidManifest.xml instead of
   ${project.basedir}/src/main/AndroidManifest.xml
   then in ManifestUpdateMojo:
3. remove updatedManifestFile (which points to
   ${project.basedir}/src/main/AndroidManifest.xml) and rely on
   androidManifestFile from the parent class
   then in UndeployMojo:
4. change it to rely on the sourceManifestFile instead of the
   androidManifestFile
   etc etc... there are probably quite a few places to check and/or tweak

Obviously this could have repercussions on quite a few people. It's worth
us figuring out the right approach to take.

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 1:46 AM, Leonid [email protected] wrote:

Also, we have a symlink at the root for one of the Eclipse users,
which ends up triggering the androidManifestFilePre4.exists() logic,
even
though it isn't in use from Maven.

That's exactly the point! If one's application has its android manifest
placed in "new default" src/main directory, then this is good; for those
apps that still keep manifest in project root for legacy reasons, AMP
will
keep firing project layout warnings unless developer explicitly turns
them
off. And if warnings are disabled, he might overlook some other issue
with
project structure, I guess.

The bottom line of this long discussion is, imho, is that having 3
parameters to configure 2 file locations looks like an overhead to me,
as
at least two parameters have to be assigned the same value to not to
break
the project. See my pull request #533
#533 for a
way
to at least partially overcome this situation.

—
Reply to this email directly or view it on GitHub
<
https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66594987>

.

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[malachid]

IMHO, the build should only modify the target/ directory

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 9:00 AM, Benoit Billington <[email protected]

wrote:

Yep I like that approach.

I'd like everyone to answer a question that was asked elsewhere:
After updating the manifest using the manifest-update or manifest-merger
mojo where would you want to have the updated manifest located? In target
or in the src?
On 11 Dec 2014 17:21, "Malachi de AElfweald" [email protected]
wrote:

Based on your comment, I relooked at my poms and found that I was in
fact
setting the updatedManifestFile in one of my profiles to point to
${project.build.directory}/AndroidManifest.xml for the update-manifest
goal.
I'm also pointing androidManifestFile to
${project.basedir}/src/main/android/AndroidManifest.xml for the undeploy
goal, since I attach it to pre-clean and the target version may not yet
exist.

I agree with you that we need to use some sensible defaults here. We
should be able to specify at most two locations and be done with it.

Looking at the pull request, I have two comments (including in this
conversation rather than in the PR itself):

1. I don't think the validateStandardLocations changes wouldn't have any
   effect on my need to use it, as I in essence have two source manifests.
   The
   original and the symlink. Maybe we need logic to whitelist rather than
   blacklist. Is the valid location there, rather than is any invalid
   location available. Of course, in my case that still wouldn't help --
   but
   it would help others.
2. Removing the default from the updatedManifestFile; but still having
   it
   specify the same property as another parameter (androidManifestFile) in
   the
   parent class...

which leads me to thinking... maybe we should change the
AbstractAndroidMojo defaults:

1. change sourceManifestFile to point to
   ${project.basedir}/src/main/AndroidManifest.xml instead of nothing
2. change androidManifestFile to point to
   ${project.build.directory}/AndroidManifest.xml instead of
   ${project.basedir}/src/main/AndroidManifest.xml
   then in ManifestUpdateMojo:
3. remove updatedManifestFile (which points to
   ${project.basedir}/src/main/AndroidManifest.xml) and rely on
   androidManifestFile from the parent class
   then in UndeployMojo:
4. change it to rely on the sourceManifestFile instead of the
   androidManifestFile
   etc etc... there are probably quite a few places to check and/or tweak

Obviously this could have repercussions on quite a few people. It's
worth
us figuring out the right approach to take.

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 1:46 AM, Leonid [email protected]
wrote:

Also, we have a symlink at the root for one of the Eclipse users,
which ends up triggering the androidManifestFilePre4.exists() logic,
even
though it isn't in use from Maven.

That's exactly the point! If one's application has its android
manifest
placed in "new default" src/main directory, then this is good; for
those
apps that still keep manifest in project root for legacy reasons, AMP
will
keep firing project layout warnings unless developer explicitly turns
them
off. And if warnings are disabled, he might overlook some other issue
with
project structure, I guess.

The bottom line of this long discussion is, imho, is that having 3
parameters to configure 2 file locations looks like an overhead to me,
as
at least two parameters have to be assigned the same value to not to
break
the project. See my pull request #533
#533 for a
way
to at least partially overcome this situation.

—
Reply to this email directly or view it on GitHub
<

https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66594987>

.

—
Reply to this email directly or view it on GitHub
<
https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66644020>

.

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[william-ferguson-au]

What Malachi said - updates should occur in target.

[image: photo]
William Ferguson
Founder and CEO, XandarMob
m:+61 425 716 870 | e:[email protected] |
w:https://wylas-timing.com
http://lexathon.com <http://wylas-timing.com http://lexathon.com>
ᐧ

On Fri, Dec 12, 2014 at 3:51 AM, Malachi de AElfweald <
[email protected]> wrote:

IMHO, the build should only modify the target/ directory

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 9:00 AM, Benoit Billington <
[email protected]

wrote:

Yep I like that approach.

I'd like everyone to answer a question that was asked elsewhere:
After updating the manifest using the manifest-update or manifest-merger
mojo where would you want to have the updated manifest located? In
target
or in the src?
On 11 Dec 2014 17:21, "Malachi de AElfweald" [email protected]
wrote:

Based on your comment, I relooked at my poms and found that I was in
fact
setting the updatedManifestFile in one of my profiles to point to
${project.build.directory}/AndroidManifest.xml for the update-manifest
goal.
I'm also pointing androidManifestFile to
${project.basedir}/src/main/android/AndroidManifest.xml for the
undeploy
goal, since I attach it to pre-clean and the target version may not
yet
exist.

I agree with you that we need to use some sensible defaults here. We
should be able to specify at most two locations and be done with it.

Looking at the pull request, I have two comments (including in this
conversation rather than in the PR itself):

1. I don't think the validateStandardLocations changes wouldn't have
   any
   effect on my need to use it, as I in essence have two source
   manifests.
   The
   original and the symlink. Maybe we need logic to whitelist rather than
   blacklist. Is the valid location there, rather than is any invalid
   location available. Of course, in my case that still wouldn't help --
   but
   it would help others.
2. Removing the default from the updatedManifestFile; but still having
   it
   specify the same property as another parameter (androidManifestFile)
   in
   the
   parent class...

which leads me to thinking... maybe we should change the
AbstractAndroidMojo defaults:

1. change sourceManifestFile to point to
   ${project.basedir}/src/main/AndroidManifest.xml instead of nothing
2. change androidManifestFile to point to
   ${project.build.directory}/AndroidManifest.xml instead of
   ${project.basedir}/src/main/AndroidManifest.xml
   then in ManifestUpdateMojo:
3. remove updatedManifestFile (which points to
   ${project.basedir}/src/main/AndroidManifest.xml) and rely on
   androidManifestFile from the parent class
   then in UndeployMojo:
4. change it to rely on the sourceManifestFile instead of the
   androidManifestFile
   etc etc... there are probably quite a few places to check and/or tweak

Obviously this could have repercussions on quite a few people. It's
worth
us figuring out the right approach to take.

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 1:46 AM, Leonid [email protected]
wrote:

Also, we have a symlink at the root for one of the Eclipse users,
which ends up triggering the androidManifestFilePre4.exists() logic,
even
though it isn't in use from Maven.

That's exactly the point! If one's application has its android
manifest
placed in "new default" src/main directory, then this is good; for
those
apps that still keep manifest in project root for legacy reasons,
AMP
will
keep firing project layout warnings unless developer explicitly
turns
them
off. And if warnings are disabled, he might overlook some other
issue
with
project structure, I guess.

The bottom line of this long discussion is, imho, is that having 3
parameters to configure 2 file locations looks like an overhead to
me,
as
at least two parameters have to be assigned the same value to not to
break
the project. See my pull request #533
#533 for
a
way
to at least partially overcome this situation.

—
Reply to this email directly or view it on GitHub
<

https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66594987>

.

—
Reply to this email directly or view it on GitHub
<

https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66644020>

.

—
Reply to this email directly or view it on GitHub
<
https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66651230>

.

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[Shusshu]

Yup I also agree with malachi.

Last time I and others preferred to have the updated version in the src.
Using the manifest-update mojo it was understandable but against mvn way of
working.

Using the manifest-merger it should only be changed in target.
On 11 Dec 2014 19:19, "William Ferguson" [email protected] wrote:

What Malachi said - updates should occur in target.

[image: photo]
William Ferguson
Founder and CEO, XandarMob
m:+61 425 716 870 | e:[email protected] |
w:https://wylas-timing.com
http://lexathon.com <http://wylas-timing.com http://lexathon.com>
ᐧ

On Fri, Dec 12, 2014 at 3:51 AM, Malachi de AElfweald <
[email protected]> wrote:

IMHO, the build should only modify the target/ directory

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 9:00 AM, Benoit Billington <
[email protected]

wrote:

Yep I like that approach.

I'd like everyone to answer a question that was asked elsewhere:
After updating the manifest using the manifest-update or
manifest-merger
mojo where would you want to have the updated manifest located? In
target
or in the src?
On 11 Dec 2014 17:21, "Malachi de AElfweald" [email protected]

wrote:

Based on your comment, I relooked at my poms and found that I was in
fact
setting the updatedManifestFile in one of my profiles to point to
${project.build.directory}/AndroidManifest.xml for the
update-manifest
goal.
I'm also pointing androidManifestFile to
${project.basedir}/src/main/android/AndroidManifest.xml for the
undeploy
goal, since I attach it to pre-clean and the target version may not
yet
exist.

I agree with you that we need to use some sensible defaults here. We
should be able to specify at most two locations and be done with it.

Looking at the pull request, I have two comments (including in this
conversation rather than in the PR itself):

1. I don't think the validateStandardLocations changes wouldn't have
   any
   effect on my need to use it, as I in essence have two source
   manifests.
   The
   original and the symlink. Maybe we need logic to whitelist rather
   than
   blacklist. Is the valid location there, rather than is any invalid

location available. Of course, in my case that still wouldn't help

but
it would help others.
2. Removing the default from the updatedManifestFile; but still
having
it
specify the same property as another parameter (androidManifestFile)
in
the
parent class...

which leads me to thinking... maybe we should change the
AbstractAndroidMojo defaults:

1. change sourceManifestFile to point to
   ${project.basedir}/src/main/AndroidManifest.xml instead of nothing
2. change androidManifestFile to point to
   ${project.build.directory}/AndroidManifest.xml instead of
   ${project.basedir}/src/main/AndroidManifest.xml
   then in ManifestUpdateMojo:
3. remove updatedManifestFile (which points to
   ${project.basedir}/src/main/AndroidManifest.xml) and rely on
   androidManifestFile from the parent class
   then in UndeployMojo:
4. change it to rely on the sourceManifestFile instead of the
   androidManifestFile
   etc etc... there are probably quite a few places to check and/or
   tweak

Obviously this could have repercussions on quite a few people. It's
worth
us figuring out the right approach to take.

Malachi de Ælfweald
http://www.google.com/profiles/malachid

On Thu, Dec 11, 2014 at 1:46 AM, Leonid [email protected]
wrote:

Also, we have a symlink at the root for one of the Eclipse users,
which ends up triggering the androidManifestFilePre4.exists()
logic,
even
though it isn't in use from Maven.

That's exactly the point! If one's application has its android
manifest
placed in "new default" src/main directory, then this is good; for
those
apps that still keep manifest in project root for legacy reasons,
AMP
will
keep firing project layout warnings unless developer explicitly
turns
them
off. And if warnings are disabled, he might overlook some other
issue
with
project structure, I guess.

The bottom line of this long discussion is, imho, is that having 3
parameters to configure 2 file locations looks like an overhead to
me,
as
at least two parameters have to be assigned the same value to not
to
break
the project. See my pull request #533
#533
for
a
way
to at least partially overcome this situation.

—
Reply to this email directly or view it on GitHub
<

https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66594987>

.

—
Reply to this email directly or view it on GitHub
<

https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66644020>

.

—
Reply to this email directly or view it on GitHub
<

https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66651230>

.

—
Reply to this email directly or view it on GitHub
<
https://github.com/simpligility/android-maven-plugin/issues/508#issuecomment-66659169>

.

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[greek1979]

After updating the manifest using the manifest-update or manifest-merger
mojo where would you want to have the updated manifest located? In target
or in the src?

A file updated during the build process should end up in the "target" directory, I believe. That's where build artifacts are being thrown into.

Hence it would be nice to have only two parameters. In a perfect world, it would be "source" (where manifest is taken from), and "destination" (where it ends up after all the modifications; target directory being the default). Later parameter is optional, as some developer may prefer to explicitly overwrite original (source) file to retain the changes. Not sure about proper names for these two, however...

[Shusshu]

So what are the proposed changes everyone agrees on? I can do the pull request but I prefer to be sure on what we all want.

Would the following be fine ?

sourceManifestFile default to src/main/AndroidManifest.xml
androidManifestFile default to target/AndroidManifest.xml
Remove updatedManifestFile

So the GenerateSourceMojo copy the manifest file from sourceManifestFile to androidManifestFile

The ManifestUpdateMojo & ManifestMergerMojo process the sourceManifestFile and place the output into androidManifestFile

Other mojos need to use androidManifestFile

[greek1979]

The changes you are proposing are fine, but would imply that everyone who develops Android apps with AMP need to review their plugin configurations. To be precise, this is the most tricky part:

Other mojos need to use androidManifestFile

If there is a Maven-built Android app that only specifies androidManifestFile in pom.xml, and it points to an actual (and final) location of manifest file, and sourceManifestFile is not explicitly defined, then such a configuration would become instantly broken. We can output a warning, though, if a manifest file referenced by sourceManifestFile was not found, but there is some file matching androidManifestFile path.

If I understand it correctly, if new parameter sourceManifestFile is not explicitly defined in plugin configuration (i.e. not present in the pom), then it would get its default value; and there is no good way to determine whether user has intentionally left the parameter out of configuration, or he simply forgot about it?

[Shusshu]

Yes it's a breaking change... AMP 4.1 or 4.5 or 5.0....

Is there a way to solve this issue without breaking all current configurations?

Most people I guess are only using androidManifestFile so we could do it either the way proposed in the #541 pull request or:

we remove sourceManifestFile, make the default location of androidManifestFile to src/main/AndroidManifest.xml and make the default location of finalManifestFile to the build directory and use that variable in all mojos.... in that case we would break the config for people using sourceManifest & updatedManifest which is fine I guess. See #542

[mosabua]

Not sure... we could also keep the parameters we dont want to use anymore around, set them as deprecated in the UI docs and break the build if a value for these is configured in the pom. Then we could release it as 4.1 with a minor API breakage.. and towards 5.0 we remove it for real. Wdyt @simpligility/android-maven-plugins-core-committers

[Shusshu]

What would be the implementation ? you would deprecate some params but would still use them? What would be the default value of each params?

[Shusshu]

I closed the wrong ticket.

So to continue on this issue.

Which approach would you guys prefer?

#541 ?

#542 ?

Or something else?

[greek1979]

If somebody would ask me, then I'd cast my vote in favor of #541 as it simplifies the things a bit, and gets all mojos aligned and working in more or less the same way. Contrary to #542 which deprecates one parameter, but introduces another, with a somewhat overlapping default value. Happy New Year everybody!

[Shusshu]

Not exactly

#541 removes the updatedManifestFile parameter and would break non standard configurations.

#542 Removes 2 parameters: sourceManifestFile & updatedManifestFile and add a new one destinationManifestFile which has a different default value than androidManifestFile. androidManifestFile would be in the src/ and destinationManifestFile would be in the target/ folder

[mosabua]

I would go with 542 from all I have seen.

[william-ferguson-au]

#542
On 30 Dec 2014 3:53 am, "Manfred Moser" [email protected] wrote:

I would go with 542 from all I have seen.

—
Reply to this email directly or view it on GitHub
#508 (comment)
.

[Shusshu]

I also think 542 is better for backwards compatibility (just break config of people using source & updated parameters) the 541 would break stuff and it would be harder for the user to understand what to do.

[mosabua]

I think thats enough votes.. go ahead and merge it ;-)