You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by "Brian E. Fox" <br...@reply.infinity.nu> on 2007/06/20 05:02:47 UTC
.isSnapshot changes version
I discovered http://jira.codehaus.org/browse/MNG-2961 while working on
mdep a while back. It should be an easy fix but I'm pondering what the
least intrusive fix is. Clearly calling a Boolean method shouldn't
result in a change to some other variable so we should do something.
Changing it to always returns xxx-SNAPSHOT as it does after calling
isSnapshot has the potential to break things depending on existing
behavior. The cleanest way forward is most likely to not have isSnapshot
modify the version, and to create a new method that returns xxx-SNAPSHOT
and leave getBaseVersion to return just the xxx-timestamp. (not sure
what this method would be called...getNonResolvedBaseVersion)
WDYT?
Re: .isSnapshot changes version
Posted by Hervé BOUTEMY <he...@free.fr>.
Le vendredi 22 juin 2007, Hervé BOUTEMY a écrit :
> Le vendredi 22 juin 2007, Kenney Westerhof a écrit :
> > * There are multiple methods for version modification/retrieval:
> >
> > * resolved version (setter only - sets version)
> > * version (sets version, baseversion, and versionrange; gets version)
> > * base version (sets base version, gets baseversion or version if
> > baseversion null) * version range (sets versionrange, and sets version
> > and baseversion either to null or a recommeded version; gets
> > versionrange) * available versions
> > * selectVersion (sets version and baseversion)
> > * updateVersion (sets version and file)
> >
> > These can be set independently and can result in an inconsistent state.
> > Documentation lacks to describe which you can and which you cannot call
> > in what sequences.
>
> Yes, this is precisely what made me very careful on any patch proposal for
> MNG-2961...
>
> > * isRelease is a boolean that has nothing to do with versions. I'd figure
> > that if something wasn't a snapshot, it'd be a release. Guess not. Or
> > maybe it is, but then the logic is someplace else.
> >
> >
> > Simplifying this should make a whole lot of difference.
>
> yes: perhaps add this to Architectural Goals for Maven 2.1?
>
> > For now, we should update the setters to immediately update baseversion
> > to -SNAPSHOT, so that even when you don't call isSNapshot, getBaseVersion
> > is ok. Just updating setBaseVersion isn't enough, but it's a start. If
> > you do setBaseVersion(timestamp) it'll discard the real value and replace
> > it with -SNAPSHOT.
>
> +1: I can provide a patch if it helps
patch attached to http://jira.codehaus.org/browse/MNG-2961
Hervé
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: .isSnapshot changes version
Posted by Hervé BOUTEMY <he...@free.fr>.
Le vendredi 22 juin 2007, Kenney Westerhof a écrit :
> * There are multiple methods for version modification/retrieval:
>
> * resolved version (setter only - sets version)
> * version (sets version, baseversion, and versionrange; gets version)
> * base version (sets base version, gets baseversion or version if
> baseversion null) * version range (sets versionrange, and sets version and
> baseversion either to null or a recommeded version; gets versionrange) *
> available versions
> * selectVersion (sets version and baseversion)
> * updateVersion (sets version and file)
>
> These can be set independently and can result in an inconsistent state.
> Documentation lacks to describe which you can and which you cannot call in
> what sequences.
Yes, this is precisely what made me very careful on any patch proposal for
MNG-2961...
>
> * isRelease is a boolean that has nothing to do with versions. I'd figure
> that if something wasn't a snapshot, it'd be a release. Guess not. Or maybe
> it is, but then the logic is someplace else.
>
>
> Simplifying this should make a whole lot of difference.
yes: perhaps add this to Architectural Goals for Maven 2.1?
>
> For now, we should update the setters to immediately update baseversion to
> -SNAPSHOT, so that even when you don't call isSNapshot, getBaseVersion is
> ok. Just updating setBaseVersion isn't enough, but it's a start. If you do
> setBaseVersion(timestamp) it'll discard the real value and replace it with
> -SNAPSHOT.
+1: I can provide a patch if it helps
Hervé
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: .isSnapshot changes version
Posted by Mark Hobson <ma...@gmail.com>.
On 22/06/07, Kenney Westerhof <ke...@apache.org> wrote:
> Well, just looked at the code and it's a mess. Here are the problems:
>
> * The isSnapshot method does a regexp on the version for the timestamp, and if it matches,
> replaces the baseversion with -SNAPSHOT. If it doesn't match, checks for 'endswith(SNAPSHOT)'.
> So this method changes the baseversion on some occasions.
>
> * The above means that calling isSnapshot on an artifact with baseversion '1SNAPSHOT' or 'SNAPSHOT' is
> considered a snapshot IFF the baseversion is not timestamp-resolved yet.
> So, it'll return true for non-timestamp resolved artifacts, but
> it'll return false if they have. This is because the regexp checks for ^(.*)-(\d\d\detc....)',
> so the timestamp has to be preceded by '-'.
>
> * LATEST is also considered a snapshot, if the version has not been resolved. If it has been resolved
> to a real version (1.0 for instance), it won't be a snapshot. I'm not sure if this is a problem,
> if only snapshots are considered in resolution. Shouldn't LATEST resolve to non-snapshots?
> Ah, they do: AbstractVersionTransformation:
>
> // Don't use snapshot metadata for LATEST (which isSnapshot returns true for)
> if ( !artifact.isSnapshot() || Artifact.LATEST_VERSION.equals( artifact.getBaseVersion() ) )
> {
> metadata = new ArtifactRepositoryMetadata( artifact );
> }
> else
> {
> metadata = new SnapshotArtifactRepositoryMetadata( artifact );
> }
>
> This needs to be fixed - i'm sure there's a reason for LATEST being treated like snapshots at some
> places, and not at later places. As soon as LATEST is resolved it's no longer a snapshot, surely this
> causes problems?
>
> * There are multiple methods for version modification/retrieval:
>
> * resolved version (setter only - sets version)
> * version (sets version, baseversion, and versionrange; gets version)
> * base version (sets base version, gets baseversion or version if baseversion null)
> * version range (sets versionrange, and sets version and baseversion either to null or a recommeded version; gets versionrange)
> * available versions
> * selectVersion (sets version and baseversion)
> * updateVersion (sets version and file)
>
> These can be set independently and can result in an inconsistent state. Documentation lacks to describe
> which you can and which you cannot call in what sequences.
>
> * isRelease is a boolean that has nothing to do with versions. I'd figure that if something wasn't a snapshot,
> it'd be a release. Guess not. Or maybe it is, but then the logic is someplace else.
>
>
> Simplifying this should make a whole lot of difference.
>
> For now, we should update the setters to immediately update baseversion to -SNAPSHOT,
> so that even when you don't call isSNapshot, getBaseVersion is ok. Just updating setBaseVersion isn't enough,
> but it's a start. If you do setBaseVersion(timestamp) it'll discard the real value and replace it with -SNAPSHOT.
>
> Ideally I'd like to NOT use the regexps to find out if something is a snapshot, but use the repository
> metadata for that. Maybe some company uses timestamps for releases, who knows..
I noticed some of this madness when I was reading the code recently
and am certainly +1 on cleaning up the API and writing some Javadoc.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: .isSnapshot changes version
Posted by Kenney Westerhof <ke...@apache.org>.
Hervé BOUTEMY wrote:
> Le mercredi 20 juin 2007, Kenney Westerhof a écrit :
>> Also see
>>
>> https://svn.apache.org/repos/asf/maven/surefire/trunk/maven-surefire-plugin
>> /src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java
>>
>> line 494.
>>
>> I tried to fix this a while back but it's a big mess and has a lot of
>> unwanted side effects.
>>
>> Accessors shouldn't change a property, but this one does because only when
>> you really need the version, it's resolved (lazy init). Though I don't
>> understand why X-SNAPSHOT needs to be resolved to the timestamped version
>> to determine wheter it is a snapshot..
>>
>> btw, baseversion is the version containing the SNAPSHOT keyword,
>> version is the one containing the timestamp-buildernumber IIRC.
> ok,
> so I think that if a transformation has to be done from xxx-timestamp into
> xxx-SNAPHOT, then it should be in the setBaseVersion() method, so the
> baseVersion attribute is directly set to xxx-SNAPSHOT and never changed
> after, even if the parameter during the call was xxx-timestamp.
>
> Is this ok to do so, or should we investigate why it is called sometimes with
> a timestamped version and not -SNAPSHOT?
Well, just looked at the code and it's a mess. Here are the problems:
* The isSnapshot method does a regexp on the version for the timestamp, and if it matches,
replaces the baseversion with -SNAPSHOT. If it doesn't match, checks for 'endswith(SNAPSHOT)'.
So this method changes the baseversion on some occasions.
* The above means that calling isSnapshot on an artifact with baseversion '1SNAPSHOT' or 'SNAPSHOT' is
considered a snapshot IFF the baseversion is not timestamp-resolved yet.
So, it'll return true for non-timestamp resolved artifacts, but
it'll return false if they have. This is because the regexp checks for ^(.*)-(\d\d\detc....)',
so the timestamp has to be preceded by '-'.
* LATEST is also considered a snapshot, if the version has not been resolved. If it has been resolved
to a real version (1.0 for instance), it won't be a snapshot. I'm not sure if this is a problem,
if only snapshots are considered in resolution. Shouldn't LATEST resolve to non-snapshots?
Ah, they do: AbstractVersionTransformation:
// Don't use snapshot metadata for LATEST (which isSnapshot returns true for)
if ( !artifact.isSnapshot() || Artifact.LATEST_VERSION.equals( artifact.getBaseVersion() ) )
{
metadata = new ArtifactRepositoryMetadata( artifact );
}
else
{
metadata = new SnapshotArtifactRepositoryMetadata( artifact );
}
This needs to be fixed - i'm sure there's a reason for LATEST being treated like snapshots at some
places, and not at later places. As soon as LATEST is resolved it's no longer a snapshot, surely this
causes problems?
* There are multiple methods for version modification/retrieval:
* resolved version (setter only - sets version)
* version (sets version, baseversion, and versionrange; gets version)
* base version (sets base version, gets baseversion or version if baseversion null)
* version range (sets versionrange, and sets version and baseversion either to null or a recommeded version; gets versionrange)
* available versions
* selectVersion (sets version and baseversion)
* updateVersion (sets version and file)
These can be set independently and can result in an inconsistent state. Documentation lacks to describe
which you can and which you cannot call in what sequences.
* isRelease is a boolean that has nothing to do with versions. I'd figure that if something wasn't a snapshot,
it'd be a release. Guess not. Or maybe it is, but then the logic is someplace else.
Simplifying this should make a whole lot of difference.
For now, we should update the setters to immediately update baseversion to -SNAPSHOT,
so that even when you don't call isSNapshot, getBaseVersion is ok. Just updating setBaseVersion isn't enough,
but it's a start. If you do setBaseVersion(timestamp) it'll discard the real value and replace it with -SNAPSHOT.
Ideally I'd like to NOT use the regexps to find out if something is a snapshot, but use the repository
metadata for that. Maybe some company uses timestamps for releases, who knows..
-- Kenney
>
> Hervé
>
>> -- Kenney
>>
>> Hervé BOUTEMY wrote:
>>> Le mercredi 20 juin 2007, Brian E. Fox a écrit :
>>>> I discovered http://jira.codehaus.org/browse/MNG-2961 while working on
>>>> mdep a while back.
>>> same for me while working on http://jira.codehaus.org/browse/MANTTASKS-18
>>>
>>>> It should be an easy fix but I'm pondering what the
>>>> least intrusive fix is.
>>> thought it would be easy too, but as you mention, it must be the least
>>> intrusive: that's the hard part.
>>>
>>>> Clearly calling a Boolean method shouldn't
>>>> result in a change to some other variable so we should do something.
>>>>
>>>>
>>>>
>>>> Changing it to always returns xxx-SNAPSHOT as it does after calling
>>>> isSnapshot has the potential to break things depending on existing
>>>> behavior. The cleanest way forward is most likely to not have isSnapshot
>>>> modify the version, and to create a new method that returns xxx-SNAPSHOT
>>>> and leave getBaseVersion to return just the xxx-timestamp. (not sure
>>>> what this method would be called...getNonResolvedBaseVersion)
>>>>
>>>>
>>>>
>>>> WDYT?
>>> creating a new method is intrusive since this new method has to be used
>>> now: in this case, I know of
>>> o.a.m.artifact.repository.layout.DefaultRepositoryLayout.pathOf(), but I
>>> suppose there are others.
>>>
>>> To avoid a change in the API, I was thinking at modifying setVersion()
>>> and setBaseVersion() to do the job actually done in isSnapshot(). But I
>>> don't really understand the semantics of setBaseVersion(): as you noted,
>>> this has the potential to break things.
>>>
>>> To make a good decision, I think the first thing is to clearly describe
>>> the semantics of version and baseVersion: perhaps it is already clearly
>>> explained somewhere, but I don't know where. Depending the result, we'll
>>> be able to decide if the API has to be extended, or only the
>>> implementation fixed (and assume if it breaks something that works now
>>> but shouldn't because baseVersion had been used where version should
>>> have).
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>>> For additional commands, e-mail: dev-help@maven.apache.org
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
>> For additional commands, e-mail: dev-help@maven.apache.org
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: .isSnapshot changes version
Posted by Hervé BOUTEMY <he...@free.fr>.
Le mercredi 20 juin 2007, Kenney Westerhof a écrit :
> Also see
>
> https://svn.apache.org/repos/asf/maven/surefire/trunk/maven-surefire-plugin
>/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java
>
> line 494.
>
> I tried to fix this a while back but it's a big mess and has a lot of
> unwanted side effects.
>
> Accessors shouldn't change a property, but this one does because only when
> you really need the version, it's resolved (lazy init). Though I don't
> understand why X-SNAPSHOT needs to be resolved to the timestamped version
> to determine wheter it is a snapshot..
>
> btw, baseversion is the version containing the SNAPSHOT keyword,
> version is the one containing the timestamp-buildernumber IIRC.
ok,
so I think that if a transformation has to be done from xxx-timestamp into
xxx-SNAPHOT, then it should be in the setBaseVersion() method, so the
baseVersion attribute is directly set to xxx-SNAPSHOT and never changed
after, even if the parameter during the call was xxx-timestamp.
Is this ok to do so, or should we investigate why it is called sometimes with
a timestamped version and not -SNAPSHOT?
Hervé
>
> -- Kenney
>
> Hervé BOUTEMY wrote:
> > Le mercredi 20 juin 2007, Brian E. Fox a écrit :
> >> I discovered http://jira.codehaus.org/browse/MNG-2961 while working on
> >> mdep a while back.
> >
> > same for me while working on http://jira.codehaus.org/browse/MANTTASKS-18
> >
> >> It should be an easy fix but I'm pondering what the
> >> least intrusive fix is.
> >
> > thought it would be easy too, but as you mention, it must be the least
> > intrusive: that's the hard part.
> >
> >> Clearly calling a Boolean method shouldn't
> >> result in a change to some other variable so we should do something.
> >>
> >>
> >>
> >> Changing it to always returns xxx-SNAPSHOT as it does after calling
> >> isSnapshot has the potential to break things depending on existing
> >> behavior. The cleanest way forward is most likely to not have isSnapshot
> >> modify the version, and to create a new method that returns xxx-SNAPSHOT
> >> and leave getBaseVersion to return just the xxx-timestamp. (not sure
> >> what this method would be called...getNonResolvedBaseVersion)
> >>
> >>
> >>
> >> WDYT?
> >
> > creating a new method is intrusive since this new method has to be used
> > now: in this case, I know of
> > o.a.m.artifact.repository.layout.DefaultRepositoryLayout.pathOf(), but I
> > suppose there are others.
> >
> > To avoid a change in the API, I was thinking at modifying setVersion()
> > and setBaseVersion() to do the job actually done in isSnapshot(). But I
> > don't really understand the semantics of setBaseVersion(): as you noted,
> > this has the potential to break things.
> >
> > To make a good decision, I think the first thing is to clearly describe
> > the semantics of version and baseVersion: perhaps it is already clearly
> > explained somewhere, but I don't know where. Depending the result, we'll
> > be able to decide if the API has to be extended, or only the
> > implementation fixed (and assume if it breaks something that works now
> > but shouldn't because baseVersion had been used where version should
> > have).
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> > For additional commands, e-mail: dev-help@maven.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: .isSnapshot changes version
Posted by Kenney Westerhof <ke...@apache.org>.
Also see
https://svn.apache.org/repos/asf/maven/surefire/trunk/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java
line 494.
I tried to fix this a while back but it's a big mess and has a lot of unwanted
side effects.
Accessors shouldn't change a property, but this one does because only when you
really need the version, it's resolved (lazy init). Though I don't understand
why X-SNAPSHOT needs to be resolved to the timestamped version to determine
wheter it is a snapshot..
btw, baseversion is the version containing the SNAPSHOT keyword,
version is the one containing the timestamp-buildernumber IIRC.
-- Kenney
Hervé BOUTEMY wrote:
> Le mercredi 20 juin 2007, Brian E. Fox a écrit :
>> I discovered http://jira.codehaus.org/browse/MNG-2961 while working on
>> mdep a while back.
> same for me while working on http://jira.codehaus.org/browse/MANTTASKS-18
>
>> It should be an easy fix but I'm pondering what the
>> least intrusive fix is.
> thought it would be easy too, but as you mention, it must be the least
> intrusive: that's the hard part.
>
>> Clearly calling a Boolean method shouldn't
>> result in a change to some other variable so we should do something.
>>
>>
>>
>> Changing it to always returns xxx-SNAPSHOT as it does after calling
>> isSnapshot has the potential to break things depending on existing
>> behavior. The cleanest way forward is most likely to not have isSnapshot
>> modify the version, and to create a new method that returns xxx-SNAPSHOT
>> and leave getBaseVersion to return just the xxx-timestamp. (not sure
>> what this method would be called...getNonResolvedBaseVersion)
>>
>>
>>
>> WDYT?
> creating a new method is intrusive since this new method has to be used now:
> in this case, I know of
> o.a.m.artifact.repository.layout.DefaultRepositoryLayout.pathOf(), but I
> suppose there are others.
>
> To avoid a change in the API, I was thinking at modifying setVersion() and
> setBaseVersion() to do the job actually done in isSnapshot(). But I don't
> really understand the semantics of setBaseVersion(): as you noted, this has
> the potential to break things.
>
> To make a good decision, I think the first thing is to clearly describe the
> semantics of version and baseVersion: perhaps it is already clearly explained
> somewhere, but I don't know where. Depending the result, we'll be able to
> decide if the API has to be extended, or only the implementation fixed (and
> assume if it breaks something that works now but shouldn't because
> baseVersion had been used where version should have).
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
> For additional commands, e-mail: dev-help@maven.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
Re: .isSnapshot changes version
Posted by Hervé BOUTEMY <he...@free.fr>.
Le mercredi 20 juin 2007, Brian E. Fox a écrit :
> I discovered http://jira.codehaus.org/browse/MNG-2961 while working on
> mdep a while back.
same for me while working on http://jira.codehaus.org/browse/MANTTASKS-18
> It should be an easy fix but I'm pondering what the
> least intrusive fix is.
thought it would be easy too, but as you mention, it must be the least
intrusive: that's the hard part.
> Clearly calling a Boolean method shouldn't
> result in a change to some other variable so we should do something.
>
>
>
> Changing it to always returns xxx-SNAPSHOT as it does after calling
> isSnapshot has the potential to break things depending on existing
> behavior. The cleanest way forward is most likely to not have isSnapshot
> modify the version, and to create a new method that returns xxx-SNAPSHOT
> and leave getBaseVersion to return just the xxx-timestamp. (not sure
> what this method would be called...getNonResolvedBaseVersion)
>
>
>
> WDYT?
creating a new method is intrusive since this new method has to be used now:
in this case, I know of
o.a.m.artifact.repository.layout.DefaultRepositoryLayout.pathOf(), but I
suppose there are others.
To avoid a change in the API, I was thinking at modifying setVersion() and
setBaseVersion() to do the job actually done in isSnapshot(). But I don't
really understand the semantics of setBaseVersion(): as you noted, this has
the potential to break things.
To make a good decision, I think the first thing is to clearly describe the
semantics of version and baseVersion: perhaps it is already clearly explained
somewhere, but I don't know where. Depending the result, we'll be able to
decide if the API has to be extended, or only the implementation fixed (and
assume if it breaks something that works now but shouldn't because
baseVersion had been used where version should have).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org