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

Remove Cygwin distro support #36778

Merged
merged 17 commits into from Dec 14, 2023
Merged

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Nov 26, 2023

Removing Cygwin support from:

  • build system,
  • CI,
  • SPKG configuration and installation scripts,
  • Sage library.

Depends on #36779

@@ -30,12 +30,5 @@ elif xbps-install --version > /dev/null 2>&1; then
elif pkg -v > /dev/null 2>&1; then
echo freebsd
else
case `uname -s` in
CYGWIN*)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep build/bin/sage-guess-package-system, build/bin/sage-print-system-package-command, and the cygwin.txt files for a while longer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other removals and changes LGTM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of keeping cygwin.txt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know but there are 100 other things needed to remove cygwin so I don't really care if we save that for last. Feel free to revert that commit or rebase it away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of keeping cygwin.txt ?

Because we have a nice little information system there, which I want to offer to other projects via #31662.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know but there are 100 other things needed to remove cygwin so I don't really care if we save that for last. Feel free to revert that commit or rebase it away.

OK, I'll split this part out to a separate PR that I'll mark "pending".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's now #36782.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep build/bin/sage-guess-package-system, build/bin/sage-print-system-package-command, and the cygwin.txt files for a while longer.

For how much longer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of keeping cygwin.txt ?

Because we have a nice little information system there, which I want to offer to other projects via #31662.

Cygwin was dragging down Sage - are you keen on offering this burden to other projects? :-)

@mkoeppe mkoeppe mentioned this pull request Nov 26, 2023
5 tasks
build/pkgs/gf2x/SPKG.rst Outdated Show resolved Hide resolved
COPYING.txt Outdated Show resolved Hide resolved
@dimpase
Copy link
Member

dimpase commented Nov 26, 2023

OK, this seems to be done; there are several Cygwin-related parts of patches left, but that's not urgent.

@mkoeppe
Copy link
Member

mkoeppe commented Nov 26, 2023

@dimpase Your force-push undid my changes discussed above, please fix

@dimpase
Copy link
Member

dimpase commented Nov 27, 2023

@dimpase Your force-push undid my changes discussed above, please fix

I'm not sure what I broke here, if anything.

@mkoeppe
Copy link
Member

mkoeppe commented Nov 27, 2023

There's a handy "Compare" button next to your force push

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me like this.

In case the cygwin.txt turn out to be handy in the future, they can be easily restored from the git history.

@mkoeppe
Copy link
Member

mkoeppe commented Nov 28, 2023

@dimpase Your force-push undid my changes discussed above, please fix

I'm not sure what I broke here, if anything.

Fixed it for you.

@@ -1777,13 +1777,12 @@ cdef init_libsingular() noexcept:
os.environ["SINGULAR_BIN_DIR"] = dirname(singular_executable)

import platform
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import platform

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 822a9f9

@mkoeppe
Copy link
Member

mkoeppe commented Nov 29, 2023

Probably should be tested on (non-conda) macOS

@orlitzky
Copy link
Contributor Author

I just re-read all of the changes. They all look relatively safe and everyone's comments have been addressed, so I'm going to set it to positive review. The sooner this gets merged the sooner I can start deleting the rest.

Copy link

github-actions bot commented Dec 6, 2023

Documentation preview for this PR (built with commit d9d8e14; changes) is ready! 🎉

@vbraun
Copy link
Member

vbraun commented Dec 10, 2023

merge conflict

@mkoeppe
Copy link
Member

mkoeppe commented Dec 10, 2023

I'll resolve the conflict

SageMath version 10.3.beta1, Release Date: 2023-12-10
@vbraun vbraun merged commit 1bbdb3e into sagemath:develop Dec 14, 2023
19 of 30 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 14, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 17, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

Most normal SPKGs are installed by staging in `DESTDIR`. When copying to
the final install location in `SAGE_LOCAL`, an installation record is
created, which is used later in package uninstallation.

However, when SPKG installation was switched to using staging in
`DESTDIR` (Meta-ticket sagemath#24024), the parts of `spkg-install` scripts that
used to be responsible for removing an old version of the installed
package were either kept in place or moved to `spkg-legacy-uninstall`
scripts. This was done to enable incremental builds from older
installations.

By passage of time, this is no longer needed.

Some of the removals are a partial cherry-pick from sagemath#25140 by @embray.

We also switch `frobby` to `DESTDIR` staging and *add* an `spkg-legacy-
uninstall` script.

Resolves sagemath#25140.
Resolves sagemath#30480.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36778 (merged here to resolve merge conflict)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36839
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik
@orlitzky orlitzky deleted the no-cygwin-distro-support branch April 14, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants