I prefer to revert the change in VersionInfoMojo. These fields 
version-info.properties are introduced on purpose in 
https://github.com/apache/storm/pull/2844 
<https://github.com/apache/storm/pull/2844> 
https://github.com/apache/storm/pull/2891 
<https://github.com/apache/storm/pull/2891> so we should probably keep it. 

It makes sense to return unknown if it’s built within git or SCM as there is no 
other way to tell. 


> On Aug 15, 2019, at 9:12 AM, Derek Dagit <da...@apache.org> wrote:
> 
> Well, the change seems to be to throw an IllegalArgumentException
> instead of what was the default behavior: to return a String "Unknown".
> 
> It is unclear how it would work outside of a Git working tree (as when
> the source is built from a download), so it seems the easiest fix would
> be to change the default labels to do nothing and revert to the previous
> behavior—if we can convince checkstyle that it is OK.
> 
> A better fix might be as you suggested. I like the idea of the build not
> behaving differently depending on if it is executed within SCM, at least
> if we can help it.
> 
> In any case, I am inclined to think that this should be fixed before the
> release, as it is a bug that prevents the code from building as per our
> instructions.
> 
> On Thu, Aug 15, 2019 at 02:08:17PM +0200, Stig Rohde Døssing wrote:
>> 
>> As far as I can tell, the version plugin is supposed to populate these
>> files
>> https://github.com/apache/storm/blob/master/storm-client/src/resources/storm-client-version-info.properties
>> and
>> https://github.com/apache/storm/blob/master/storm-core/src/resources/storm-core-version-info.properties.
>> I'm not sure why we need this file in both storm-client and storm-core, I'm
>> not sure anything reads the storm-core file, so maybe we can delete it?
>> 
>> When the build runs, those files are supposed to be populated with version
>> info (commit hash, author info and so on) from the plugin. The version info
>> is then read into
>> https://github.com/apache/storm/blob/925422a5b5ad1c3329a2c2b44db460ae94f70806/storm-client/src/jvm/org/apache/storm/utils/VersionInfo.java.
>> The VersionInfo file also loads the Storm version from the pom, where it's
>> used to populate e.g. the version in Storm UI for Nimbus and Supervisors.
>> 
>> It looks like this:
>> 
>> version=2.0.0
>> revision=2ba95bbd1c911d4fc6363b1c4b9c4c6d86ac9aae
>> branch=(no branch)
>> user=tgoetz
>> date=2019-04-29T21:17Z
>> url=https://git-wip-us.apache.org/repos/asf/storm.git
>> srcChecksum=58779abf76d8a8bbd20a2bd5f466
>> 
>> The only field I see read is the Storm version from the pom, I'm not sure
>> we use the other stuff (e.g. commit hash, commit author). For 2.0.0
>> something broke when splitting out storm-client, so the file for that
>> module doesn't have all fields populated:
>> 
>> version=2.0.0
>> revision=${version-info.scm.commit}
>> branch=${version-info.scm.branch}
>> user=tgoetz
>> date=${version-info.build.time}
>> url=${version-info.scm.uri}
>> srcChecksum=${version-info.source.md5}
>> 
>> It's most likely just because storm-client is missing configuration to run
>> the plugin when building.
>> 
>> I think if we're not using these extra fields from git, we might as well
>> drop the version info mojo. If we only need to set the "version" field, we
>> don't need to invoke git during the build.
>> 
>> If we do need these fields for something, we should just make the mojo set
>> the fields to "Unknown" if it fails to invoke git (i.e. if SCM.NONE is
>> set). I'm guessing what's happening for Derek is that he downloaded the
>> sources and is trying to run the build with files that aren't part of a git
>> repo. So when the mojo tries to do "git branch" it gets an error.
>> 
>> What do you think?
>> 
>> Den tor. 15. aug. 2019 kl. 06.02 skrev Ethan Li <ethanopensou...@gmail.com>:
>> 
>>> Derek ran into this error when building from source:
>>> 
>>> [ERROR] Failed to execute goal
>>> org.apache.storm:storm-maven-plugins:2.1.0:version-info (version-info) on
>>> project storm-core: java.lang.IllegalArgumentException: SCM NONE is not
>>> supported -> [Help 1]
>>> 
>>> 
>>> The log before this is
>>> 
>>> [INFO] --- storm-maven-plugins:2.1.0:version-info (version-info) @
>>> storm-core ---
>>> [WARNING] [svn, info] failed with error code 1
>>> [WARNING] [git, branch] failed with error code 128
>>> [INFO] SCM: NONE
>>> 
>>> 
>>> I compared with apache-storm-2.0.0 and found that it can pass although
>>> there are similar WARNINGs:
>>> 
>>> [INFO] --- storm-maven-plugins:2.0.0:version-info (version-info) @
>>> storm-core ---
>>> [WARNING] [svn, info] failed with error code 1
>>> [WARNING] [git, branch] failed with error code 128
>>> [INFO] SCM: NONE
>>> [INFO] Computed MD5: 58779abf76d8a8bbd20a2bd5f466
>>> 
>>> This is the PR that changed the behavior
>>> https://github.com/apache/storm/pull/3061/files#diff-f1197661326b04376e94e3b25cc3eedcR197
>>> <
>>> https://github.com/apache/storm/pull/3061/files#diff-f1197661326b04376e94e3b25cc3eedcR197>.
>>> I am not sure what this storm-maven-plugin should do though. Will need to
>>> look into it. Or if anyone has better idea, please let me know.
>>> 
>>> 
>>> 
>>>> On Aug 14, 2019, at 6:53 PM, P. Taylor Goetz <ptgo...@gmail.com> wrote:
>>>> 
>>>> Nice work Ethan!
>>>> 
>>>> I’ll try to take a look in the next few days.
>>>> 
>>>> -Taylor
>>>> 
>>>>> On Aug 14, 2019, at 2:13 PM, Ethan Li <etha...@apache.org> wrote:
>>>>> 
>>>>> This is a call to vote on releasing Apache Storm 2.1.0 (rc2)
>>>>> 
>>>>> Full list of changes in this release:
>>>>> 
>>>>> 
>>> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.1.0-rc2/RELEASE_NOTES.html
>>>>> 
>>>>> The tag/commit to be voted upon is v2.1.0:
>>>>> 
>>>>> 
>>> https://gitbox.apache.org/repos/asf?p=storm.git;a=tree;h=6c7adb8c7095cb718d5c49d07331034ae0e8434a;hb=f875619fc1017e401d65bd2900b195738f90e754
>>>>> 
>>>>> The source archive being voted upon can be found here:
>>>>> 
>>>>> 
>>> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.1.0-rc2/apache-storm-2.1.0-src.tar.gz
>>>>> 
>>>>> Other release files, signatures and digests can be found here:
>>>>> 
>>>>> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.1.0-rc2
>>>>> 
>>>>> The release artifacts are signed with the following key:
>>>>> 
>>>>> https://www.apache.org/dist/storm/KEYS
>>>>> 
>>>>> The Nexus staging repository for this release is:
>>>>> 
>>>>> https://repository.apache.org/content/repositories/orgapachestorm-1085/
>>>>> 
>>>>> Please vote on releasing this package as Apache Storm 2.1.0.
>>>>> 
>>>>> When voting, please list the actions taken to verify the release.
>>>>> 
>>>>> This vote will be open for at least 72 hours.
>>>>> 
>>>>> [ ] +1 Release this package as Apache Storm 2.1.0
>>>>> [ ]  0 No opinion
>>>>> [ ] -1 Do not release this package because...
>>>>> 
>>>>> Thanks to everyone who contributed to this release.
>>>>> 
>>>>> Ethan
>>> 
>>> 

Reply via email to