Oops, was wrong. The revision stuff is also being read by Storm UI. Yes, we should just fix VersionInfoMojo to handle running outside SCM. I'll put up a PR to fix it shortly. I think we can also delete the version info file in storm-core. I can't think of any reason why storm-core would be on the classpath, but storm-client wouldn't be.
Den tor. 15. aug. 2019 kl. 16.24 skrev Ethan Li <[email protected]>: > Sorry for typos. I meant "If it’s not built within git or SCM" > > > On Aug 15, 2019, at 9:23 AM, Ethan Li <[email protected]> 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 <[email protected] <mailto: > [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 > < > 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 > >>>> > >>>> > > > >
