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 > > > >
