Sorry for typos. I meant "If it’s not built within git or SCM"

> On Aug 15, 2019, at 9:23 AM, Ethan Li <ethanopensou...@gmail.com> wrote:
> 
> 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 
>> <mailto: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
>>>  
>>> <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