Merge the tested fixes between version 1.10.3 and 1.11.0.

sagemath / cysignals

cysignals: interrupt and signal handling for Cython

GNU Lesser General Public License v3.0

44 stars 23 forks source link

Merge the tested fixes between version 1.10.3 and 1.11.0. #161

Closed kliem closed 2 years ago

kliem commented 2 years ago

@mkoeppe, @dimpase

Unfortunatly, the new release 1.11.0 (or 1.11.1 on pypi) was rejected by Volker Braun: https://trac.sagemath.org/ticket/32576#comment:27

We cherry-pick the unproblematic changes since 1.10.3. At the time of the release of 1.10.3, the master branch was unclean as reported on #131. (I read this, but completely forgot about it, when preparing the current release, expecting that the master branch is somewhat stable).

In particular we ignore the following commits:

9e999920e45d3ce13b2e60e06b412868d36f84af
a429bd8ddd7f4f27b973694fb14010ac8e9cb10a
84cf70f6d5a9f25ce9e4741fc071d749f437cd77
9708aad2724530296ef78b658848a60623df67d2
5e745433e38a316f7f3886405a51f468c3914967

I would propose to abandon the current master branch and rename it and move the current branch to main/master.

What do you think?

dimpase commented 2 years ago

We defnitely need to fix this for Sage 9.5 one way or another.

kliem commented 2 years ago

I'm trying to reproduce Volkers failure here, by adding cypari test suite: https://github.com/kliem/cysignals/pull/7

I completely forgot about this and just assumed that the master branch was clean...

I have no preferences, how to fix it. However, as I really don't understand assembly and apparently it seems not to work, I would just remove it.

dimpase commented 2 years ago

I guess I read https://trac.sagemath.org/ticket/32576#comment:27 wrongly, I thought you mean 1.11 dropped, by mistake, assembly code, and that's why it's broken.

dimpase commented 2 years ago

if assembly was added by mistake, just remove it, sure.

kliem commented 2 years ago

https://github.com/kliem/cysignals/runs/4468720468?check_suite_focus=true

is in fact able to reproduce the failure by Volker Braun. Let's see if it goes away in the current branch.

kliem commented 2 years ago

Ok, the current tests look better than https://github.com/kliem/cysignals/pull/7/checks.

I could also run the tests on 1.10.3 (merging only the github actions) to make sure we didn't introduce a regression.

dimpase commented 2 years ago

I could also run the tests on 1.10.3 (merging only the github actions) to make sure we didn't introduce a regression.

Have you done this? (it's not clear to me from branch names what is what)

kliem commented 2 years ago

No. I haven't done this.

The branch 1.10.x_merge is the new candidate for the master branch.
The branch 1.10.x is release 1.10.3.
The branch master basically 1.11.0, which introduced a regression due to unportable assembly code.

I added cypari2 tests in the github workflows for https://github.com/kliem/cysignals/pull/7/checks to recover the reason Volker Braun rejected the current release.

kliem commented 2 years ago

For comparison I started a workflow that shows the test suite situation with 1.10.3:

https://github.com/kliem/cysignals/pull/8/checks

kliem commented 2 years ago

According to the test results, the branch 1.10.x_merged is an improvement in comparison to 1.10.3.

So, can I go ahead and replace the master branch and proceed with making a release candidate for 1.11.2?

dimpase commented 2 years ago

Sure

kliem commented 2 years ago

main has been updated accordingly.