galaxy-iuc / standards

Documentation for standards and best practices from the Galaxy IUC
http://galaxy-iuc-standards.readthedocs.io/en/latest/
6 stars 16 forks source link

Proposal: recommend defining a @VERSION_SUFFIX@ token in addition to a @TOOL_VERSION@ token #59

Closed simonbray closed 3 years ago

simonbray commented 4 years ago

Motivation: we are currently working on a planemo autoupdate command which automatically checks if newer versions of the <requirements> exist and updates them if necessary. To simplify things we want to assume that all tools have both a @TOOL_VERSION@ and @GALAXY_VERSION@ token specified (either in the tool xml file itself or in a macro).

It would be nice if this could also be an official IUC recommendation - any comments are welcome.

mvdbeek commented 4 years ago

GALAXY_VERSION sounds like it would refer to the galaxy version, how about POST_VERSION ? Do you think there would be a way to make the token optional ? If we can avoid having to define a token that's just used once I'd be strongly in favor of that -- maybe we can start looking at the autoupdate implementation and then see if that would be possible ?

simonbray commented 4 years ago

GALAXY_VERSION sounds like it would refer to the galaxy version, how about POST_VERSION ?

I think POST_VERSION would be fine.

Do you think there would be a way to make the token optional ? If we can avoid having to define a token that's just used once I'd be strongly in favor of that -- maybe we can start looking at the autoupdate implementation and then see if that would be possible ?

Yes, I see your point. I'll make a PR over the next couple of days to the planemo repo, thanks.

bgruening commented 4 years ago

xref: https://github.com/galaxyproject/planemo/pull/1065

bwlang commented 4 years ago

What does POST_VERSION refer to? like HTTP post? or POST as in "after" or something else?

hrhotz commented 4 years ago

Instead of POST_VERSION (GALAXY_VERSION): "WRAPPER_VERSION"

mvdbeek commented 4 years ago

POST_VERSION refers to post-release version, which is what we're using so that the sort order is correct according to PEP440 (demarcated by +). WRAPPER_VERSION to me sounds like the version of the wrapper, as in <tool id="cat1", version="@WRAPPER_VERSION@">.

hrhotz commented 4 years ago

ok - I might have misunderstood the question - sorry for making noise

peterjc commented 4 years ago

I would lean to having the wrapper version being required (integer, default 0), this to me is more consistent - and would make automated updates a little bit easier?

And I also prefer @POST_VERSION@ over @GALAXY_VERSION@ for the wrapper specific integer post-script:

<tool id="cat1", version="@TOOL_VERSION@+galaxy@POST_VERSION@">

I'm hoping to do some maintenance updates on my own wrappers, so adopting convention first would be timely.

bwlang commented 4 years ago

seeing the example from @peterjc made me think of @VERSION_SUFFIX@ does that address the "POST what" question?

peterjc commented 4 years ago

I like Brad's suggestion best so far for naming the new variable:

<tool id="cat1", version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@">
nsoranzo commented 4 years ago

@simonbray It seems we are reaching a consensus, do you want to open a PR updating docs/best_practices/tool_xml.rst ?

simonbray commented 4 years ago

@nsoranzo, I've updated the PR.

That said, since opening this PR I reimplemented the autoupdate command so a specifically named token is no longer required.

simonbray commented 4 years ago

If we can avoid having to define a token that's just used once I'd be strongly in favor of that

also not sure if this is still considered an issue

bgruening commented 4 years ago

So do we skip the @VERSION_SUFFIX@ and recommend to use an integer starting with 0?

simonbray commented 4 years ago

So do we skip the @VERSION_SUFFIX@ and recommend to use an integer starting with 0?

Either is fine.

bgruening commented 4 years ago

@galaxy-iuc/iuc any other comments. Should we not recommend using @VERSION_SUFFIX@ but mention it in case people want to use it?

peterjc commented 4 years ago

I think if it would only be used once, defining the @VERSION_SUFFIX@ could be optional. Is that a good compromise?

nsoranzo commented 4 years ago

Looking again at it, the proposed @VERSION_SUFFIX@ macro would encourage to unnecessarily increase the tool version suffix for all tools which are part of a suite even when only one tool is actually modified. This leads to Galaxy having to load more tools than needed.

peterjc commented 4 years ago

@nsoranzo if the maintainers do a lot of point releases for a suite that only alter a few of the tools, and the Galaxy administrator installs all of the updates, then yes.

On the other hand, implementing the suffix for each tool in a suite separately is a pain, and also the risk with shared macro files is tools in a suite not getting a version bump when they should, which makes reproducibility much harder to track.

We don't have anything in the guidelines about versioning tools in a tool suite do we?

nsoranzo commented 4 years ago

We don't have anything in the guidelines about versioning tools in a tool suite do we?

I don't think so.

simonbray commented 3 years ago

Any other comments here? I updated the text to make clear that using the @VERSION_SUFFIX@ token is optional. I personally think it should be left up to the user whether to define the token in a macro or in the individual wrapper file.

These tokens are already very widely used and it would be good to at least acknowledge their existence in the IUC standard (and guide users towards using a single macro name rather than the 4+ different options I can see currently in the tools-iuc repo).

jj-umn commented 3 years ago

approved

bgruening commented 3 years ago

Wuhu, thanks a lot everyone, especially @simonbray for pushing this! Can't wait for the bot to take action :)