Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport fixes from PR #210. #214

Merged
merged 10 commits into from
Dec 14, 2023
Merged

Backport fixes from PR #210. #214

merged 10 commits into from
Dec 14, 2023

Conversation

lohedges
Copy link
Contributor

@lohedges lohedges commented Dec 4, 2023

This PR backports the fixes from #210 into main.

  • I confirm that I have merged the latest version of main into this branch before issuing this pull request (e.g. by running git pull origin main): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

@lohedges lohedges added the bug Something isn't working label Dec 4, 2023
@lohedges
Copy link
Contributor Author

lohedges commented Dec 4, 2023

CI is failing since it looks like some of the Sire fixes were PRed as features, thus never backported. In particular, this and this.

@chryswoods
Copy link
Contributor

Sorry - I don't know what I was thinking when I made them features... Do you still want them backported, or is it ok to have these fixes in just 2023.5.0?

@chryswoods
Copy link
Contributor

I see what happened - I had them as issue branches, but combined several together into the merge into devel with a feature, as I wanted to save the CI. Because they went in merged with the feature, I forgot to backport the individual fixes to main.

@lohedges
Copy link
Contributor Author

lohedges commented Dec 8, 2023

No problem, it's easily done. If I don't do them immediately then I forget. I'm happy to wait until the release unless it's needed by Exs before, e.g. perhaps they will be using 2023.4 for a bit and need a working version. I wasn't planning on doing another BioSimSpace patch release. At present, I think the existing main branch of BioSimSpace is broken, since there is no compatible version of Sire, i.e. I merged in my fixes assuming that the Sire updates to which they were related were already backported. Guess it might be worth checking with @xiki-tempula.

@chryswoods
Copy link
Contributor

I looked at it and it wasn't that much work. Each issue fix was quite self-contained.

I've pulled in all of the issue fixes into this PR.

I think there is enough to make it worth releasing a 2023.4.2 at the same time as 2023.5.0. I guess @xiki-tempula that this would make it easier for you to have both a "latest fix update" patch release for the last major version that goes along with the first release of the next major version.

@lohedges
Copy link
Contributor Author

lohedges commented Dec 8, 2023

Thanks. I actually only spotted this because I accidentally ran CI for the PR. I normally don't for trivial backports since it's already been validated on devel. It was only this that made me realise that Sire was out of sync with the BioSimSpace fixes.

@xiki-tempula
Copy link
Contributor

Thanks for mentioning me. So we are currently basing our main to the Sire main (as we need to some fix from the main), which is not the Sire 2023.4.1. I wonder when you make them sync, would it cause trouble if we are using the main (potentially 2023.4.2)? Thanks.

@lohedges
Copy link
Contributor Author

I don't think there will be any issue. Currently BIoSimSpace main is broken since there are backports that rely on functionality that isn't in Sire main. (These are minor updates to do with rotatiing boxes and the Sire trajectory analysis.) Once these are merged into Sire main, then all is okay.

@chryswoods
Copy link
Contributor

It shouldn't - the PR for the fixes into main passes all CI. If @lohedges is happy, I will merge that in today and will prepare the 2023.4.2 release. We can then merge this PR into main on BioSimSpace and maybe do a patch release too?

@xiki-tempula
Copy link
Contributor

Great. Thanks.

@lohedges
Copy link
Contributor Author

Great. I'm off for part of the day but will try to review when I'm back. I've stacked a few other backports into this PR too. @xiki-tempula, do you want this one added too. (The one related to the AMBER mask.)

@xiki-tempula
Copy link
Contributor

Yes, please. Thanks.

@lohedges lohedges temporarily deployed to biosimspace-build December 14, 2023 09:06 — with GitHub Actions Inactive
@lohedges
Copy link
Contributor Author

All passing now that I'm building against Sire 2023.4.2 👍 I'd just like to figure out what to do about the backport for #226, then I'll be able to create a BioSimSpace 2023.4.1 release. @chryswoods, do you have any opinions on that?

@lohedges lohedges merged commit f273b68 into main Dec 14, 2023
@lohedges lohedges deleted the backport_210 branch December 14, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants