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 <[email protected]> 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 <[email protected]>: >> >>> 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 <[email protected]> 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 <[email protected]> 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 >>> >>>
