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

dist.yml: Download optional/experimental tarballs for GitHub Release assets #37762

Merged
merged 8 commits into from
May 12, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 7, 2024

Test run: https://github.com/mkoeppe/sage/actions/workflows/dist.yml

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 7, 2024

https://github.com/mkoeppe/sage/actions/runs/8585436817/job/23526820138#step:9:864

Total file count: 368 ---- Processed file #367 (99.7%)
[Error: ENOSPC: no space left on device, write] {
  errno: -28,
  code: 'ENOSPC',
  syscall: 'write'
}

@mkoeppe mkoeppe force-pushed the download-optional branch from 729fbd5 to d3e0537 Compare April 7, 2024 05:51
@mkoeppe mkoeppe changed the title .github/workflows/dist.yml: Download optional/experimental tarballs dist.yml: Download optional/experimental tarballs for GitHub Release assets Apr 7, 2024
Copy link

github-actions bot commented Apr 7, 2024

Documentation preview for this PR (built with commit a055882; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe mkoeppe requested a review from vbraun April 7, 2024 18:19
@mkoeppe mkoeppe force-pushed the download-optional branch from d3e0537 to e93c25b Compare April 27, 2024 20:20
@mkoeppe mkoeppe force-pushed the download-optional branch from e93c25b to b600e85 Compare May 6, 2024 19:52
@mkoeppe mkoeppe force-pushed the download-optional branch from b600e85 to 84c35f7 Compare May 6, 2024 20:48
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 6, 2024

Removed the fix for the symengine_py upstream_url here; it's now on #37726.

@kwankyu
Copy link
Collaborator

kwankyu commented May 10, 2024

Are these Release assets distributed from somewhere else or just from the github site?

Is it okay to bloat release assets? Some people may not prefer swollen tarballs with optional and experimental packages. I don't know as I never downloaded the tarballs for my use...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 10, 2024

Are these Release assets distributed from somewhere else or just from the github site?

All these files are also on our mirrors.

Making them available as release assets makes it faster and more robust for users to download the files because of the use of GitHub's CDN.

Is it okay to bloat release assets? Some people may not prefer swollen tarballs with optional and experimental packages. I don't know as I never downloaded the tarballs for my use...

This PR does not change what is in the large self-contained Sage release tarball. The tarball will still only contain the upstream tarballs of standard packages.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 10, 2024

Per https://docs.github.com/en/repositories/releasing-projects-on-github/about-releases#storage-and-bandwidth-quotas:

Each file included in a release must be under 2 GiB. There is no limit on the total size of a release, nor bandwidth usage.

@kwankyu
Copy link
Collaborator

kwankyu commented May 10, 2024

Are these Release assets distributed from somewhere else or just from the github site?

All these files are also on our mirrors.

You mean that the files contained in the Release assets, not the Release assets themselves?

This PR just makes Release assets bigger by adding optional (not huge) and experimental packages. So if someone was using the Release assets, then he would experience longer download time after this PR. Can you estimate how much longer?

Anyway, I agree that the Release assets don't need to be the same with the Sage release tarball...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 10, 2024 via email

@kwankyu
Copy link
Collaborator

kwankyu commented May 10, 2024

Ah, I see. Thanks.

@kwankyu
Copy link
Collaborator

kwankyu commented May 10, 2024

"Release assets" are the files that you see listed in a GitHub release such as https://github.com/sagemath/sage/releases/tag/10.3

I was only looking at release_dist artifact. Nice to see the Release assets :-)

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 10, 2024

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
… for GitHub Release assets

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- Fixes sagemath#37752, at least for the non-"huge" tarballs (we skip the "huge"
tarballs here).
- More options for `sage -package download` (a subset of what `sage
-package list` supports)
- Also remove some zombie package files

Test run: https://github.com/mkoeppe/sage/actions/workflows/dist.yml

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- Depends on sagemath#37099 (merged to resolve merge conflict)
    
URL: sagemath#37762
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
… for GitHub Release assets

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

- Fixes sagemath#37752, at least for the non-"huge" tarballs (we skip the "huge"
tarballs here).
- More options for `sage -package download` (a subset of what `sage
-package list` supports)
- Also remove some zombie package files

Test run: https://github.com/mkoeppe/sage/actions/workflows/dist.yml

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

- Depends on sagemath#37099 (merged to resolve merge conflict)
    
URL: sagemath#37762
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit 045f065 into sagemath:develop May 12, 2024
25 of 36 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GH Actions: Also put optional tarballs in the GH release assets
3 participants