ExtensionIndex 4.8 branch is using master for many (most?) extensions

extensions

(Andrey Fedorov) #1

I noticed that many of the extensions in the ExtensionsIndex 4.8 branch are using master for the extension repo. I initially thought I missed this, but it applies to many popular extensions. This is a big issue considering the breaking changes in Slicer post-4.8.

I am assuming the 4.8 branch was just forked from the master of ExtensionsIndex? Should we establish a process to automatically update all references to master to the latest hash at the time ExtensionsIndex release branch is forked, or at least ask the extensions developers to update the index files?

Problem is, right now it is not straightforward to find the latest hash that was working and was tested against 4.8 branch of Slicer.

8-/


(Andras Lasso) #2

For the extensions that we maintain, we try to keep them compatible with both 4.8 and master as long as it is possible. It is not by mistake but to reduce maintenance effort: we don’t have to keep applying fixes and improvements to two branches. About half of our extensions still use the same branch for 4.8 and master version of Slicer, but as there are more and more differences, probably eventually all of them will have two branches.


(Andrey Fedorov) #3

I see. On our side, it was definitely not intentional. We just missed the update in the 4.8. For some of the extensions, switch to the updated DCMTK is breaking due to changes in the API, and in other extensions there are modifications/improvements which are not yet working, and the result is that the extension is broken both in the nightly and latest stable.

I think it is also important for managing user expectations - master by definition is the place for experiments. It is often not possible to test under the conditions of the dashboard before committing to master, and once you committed, and the result is not working, extension will be broken in stable, which is, well, NOT stable.


(Jean Christophe Fillion Robin) #4

Choosing to use master branch instead of explicitly updating the hash implies great responsibilities, that is why this is not the default.

Problem is, right now it is not straightforward to find the latest hash that was working and was tested against 4.8 branch of Slicer.

Exactly, using master branch prevents from doing any forensic and understanding when things break.


(Andrey Fedorov) #5

No question - we screwed up big time, we are to blame, and we have to fix it.

But still, automatic replacement of all references to master with a current (at the time release is cut) master hash would be relatively easy to implement, and would probably improve user experience.


(Jean Christophe Fillion Robin) #6

:+1: This would be great


(Csaba Pinter) #7

I agree with Andrey, updating the extension hashes to the one that was latest at the time of releasing Slicer stable would be an easy solution for this. For extensions that are actively maintained, this hash fixation would make sure that the extension works for the stable, which they can update at their own leisure later. For extensions that are broken… they will remain broken either way.


(Andrey Fedorov) #8

I am glad we are in agreement!

I will take the initial pass on the script and make the PR to the ExtensionsIndex repo.


(Jean Christophe Fillion Robin) #9

To summarize, the release checklist could include an extra step:

  • Trigger build for Slicer release
  • Next day, on one of the factory, run script updateExtensionIndex in extension index build directory.

Pseudo code for updateExtensionIndex script:

Input parameters are:
    extensionindex_build_dir .. : Contains a checkout of the extension index
                                                 as well as all source and build dir of each extensions
    release  ...................: Version to associate with the next extension index release branch

(0) Set extensonindex_dir based on extensionindex_build_dir

(1) Get ${extensionName} associated with all ${extensonindex_dir}/*.s4ext

(2) for each ${extensionName}
        cd ${extensionName}
        latest_scmrevision=$(git rev-list -n1 HEAD)
        sed -e "s/scmrevision.*/scmrevision ${latest_scmrevision}/" -i ${extensionName}.s4ext

(3) cd ${extensonindex_dir}

(4) git checkout -b master-${release}

(5) git add -A

(6) git commit -m "Ensure all description files references a specific revision"   # done by slicerbot user

(7) git push origin master-${release}

(Jean Christophe Fillion Robin) #10

Assumptions for the script would be:

  • all extension source were successfully checked out and are using git

  • extension maintainer will be responsible to update the file to use master-X.Z or similar afterward


(Andrey Fedorov) #11

PR for discussion/testing etc: https://github.com/Slicer/ExtensionsIndex/pull/1529


(Andrey Fedorov) #12

@jcfr the assumptions you make are not compatible with what I had in mind.

What I had in mind is a script that does not assume that all extensions are checked out, and leave it to the person cutting the release to run it after the branch is created.

I think at the time release is prepared, the authority should be with the release maintainer. The release process should not be interrupted by lack of response from the extension maintainer. I also think that it is the courtesy to the future user to “freeze” the functionality of the extension as the default behavior as it was at the time of the release. Advanced extension developers can always revert back.


(Jean Christophe Fillion Robin) #13

leave it to the person cutting the release to run it after the branch is created.

Makes sense.

When the script will be finalized and tested (e.g on a fork of the extension index), I suggest we update this step of the release process

:+1:


(Andrey Fedorov) #14

Great, thanks! I will revise the script to add some documentation comments, will test with a fork, and will update the issue then.

Also, as I was going through this process, I discovered that some extension use scmversion release which is a misnomer considering Slicer release process. We should probably follow up with issues for those extensions to resolve this ambiguity…

fedorov@radiobeat [18:22:14] [~/github/ExtensionsIndex] [master]
% grep release *
DTI-Reg.s4ext:scmrevision release
DTIAtlasBuilder.s4ext:scmrevision release
DTIAtlasFiberAnalyzer.s4ext:scmrevision release
DTIPrep.s4ext:scmrevision release
DTIProcess.s4ext:scmrevision release
DatabaseInteractor.s4ext:scmrevision release
SPHARM-PDM.s4ext:scmrevision release
ShapePopulationViewer.s4ext:scmrevision release
ShapeVariationAnalyzer.s4ext:scmrevision release

(Andrey Fedorov) #15

I updated the PR, it is ready for review now. Once merged, I will update the wiki instructions.


(Andrey Fedorov) #16

@lassoan @jcfr

I just realized the PR I proposed to help addressing this issue was never reviewed/acted upon, and we’ve just had another release where extensions in the release branch are still using “master” for their versions (e.g., see QuantitativeReporting in 4.10 here).

Given we reached consensus back in Spring, can you consider this PR?

Whatever is the decision on the PR, we should definitely fix references to master in the extensions included in https://github.com/Slicer/ExtensionsIndex/tree/4.10!


(Andras Lasso) #17

Projects that are not maintained anymore will not get new revisions and therefore remain compatible with latest stable. Actively maintained projects are assumed to maintain their ExtensionsIndex entries, too. When versions diverge, then typically separate branch is created (hash is not used even then).

Custom Slicer applications do not use the ExtensionsIndex but all extension versions are listed in the main application repository.

Pinning specific git hashes would only help developers, who use branch name in ExtensionsIndex, maintain their extensions, introduce backward incompatible changes, and forget about creating a separate stable branch in their extension repository. This is probably affects just a few extensions.

Anyway, I think it may still worth pinning extension versions using hashes, as it seems simple to do and I don’t see any disadvantages.


(Andrey Fedorov) #18

Except when there are changes that are not backwards compatible, which did happen for us as I discussed in one of the earlier posts (backwards incompatible API change in DCMTK).

I would not assume that. People forget. I did forget about the need to freeze hash or make a branch once the release was cut, and I don’t think there was any reminder sent out. But hey - I may well be an outlier! :slight_smile: It may well be other developers have longer memory.

I do not have any data to support or refute that statement.

Anyway, enough trying to “save the world”! For the sake of expediency, I am just going to update the extensions I maintain, and send reminders to the developers for the extensions I collaborated on.


(Andras Lasso) #19

How it is possible? If you don’t make any incompatible change in your extension then it will not break. You can keep using the same branch for both master and stable, and users will keep getting updates. If course things can change in the master branch, which may force you to make changes, but you may still manage that in the same code (e.g., using version checks/ifdefs).

If you are not sure if a change is backward-compatible and don’t have the capacity to test it, then you may decide to stop maintaining the stable branch. I would still not freeze it with a hash but instead create a stable branch in your repository and writing that branch name in the ExtensionsIndex. It would be very clear in your repository (much more explicit than some git hash in another repository) and you could also backport any fixes easily into this branch.