-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Do not generate pipestatus from spkg/install #10970
Comments
Attachment: trac_10970_do_not_generate_pipestatus.patch.gz Patch to SAGE_ROOT repo |
comment:2
Patch applied, then patched file moved to new source directory. 'make testlong' passed all tests. Positive review. |
Reviewer: Mariah Lenox |
comment:3
I've recently played around with automatic testing upgrades and I think this might break upgrades. Its really a bigger issue, the sage_root repository is only upgraded rather late in the upgrade process. I think this needs to be fixed first. I will do some more testing when I have time. I'll set this ticket to "needs info" until I (or somebody else) tests upgrading. |
comment:4
My guess is that if this causes problems with upgrading, it should only happen when upgrading from versions of Sage which don't already include the file "pipestatus", which would mean some version between 4.4.4 (which doesn't have it) and 4.5.3 (which does). I haven't actually tested anything, though. Maybe the root repository should be upgraded earlier. In an install from scratch, it has to be installed somewhat late because it depends on mercurial, but when upgrading, mercurial should already be present. This would require altering the upgrade process so that it doesn't just run spkg/install. We could install the root repo by hand (in sage-upgrade) before running spkg/install. |
comment:5
Replying to @jhpalmieri:
Those were exactly my thoughts, too. I was mostly motivated by #11073. We can't stuff all files in the spkg/base repo into a here document :-) Do we have an official policy on how old a Sage version should still be able to upgrade? |
comment:7
Replying to @vbraun:
I declare "1 year" as the official policy, since that's the official policy for deprecation. I think it is a reasonable amount of time. Since sage-4.5.3 had pipestatus, and was released 2010-09-08 and the next sage release won't be for two weeks (or so), it is fine to deprecate upgrading from pre sage-4.5.3 releases. -- William |
comment:8
|
comment:9
I mentioned this to people at Sage Days and they point out that "sage -upgrade" rarely works anyways, so we shouldn't be be too worried about breaking "sage -upgrade". |
comment:10
Replying to @williamstein:
We do test upgrading on various systems from various Sage versions and we almost never have problems. Anothe question is: is upgrading really used? I mean, do normal Sage users sometimes do |
comment:11
Replying to @jdemeyer:
I use |
comment:12
I did some testing of this... and I say: positive review. |
Changed keywords from none to sd32 |
comment:14
Replying to @williamstein:
We should be worried that (or why they feel) upgrading doesn't work for them though. I guess they just didn't try since last year (when it started working better). ;-) However, I see no real point in (or any compelling reason for) removing If we do so, despite of that it [just] breaks upgrading from older releases, If at all, we IMHO should remove it in some "major" release, like e.g. Sage 5.0, or perhaps Sage 4.8, since people are probably less likely to upgrade to such a version (with |
comment:15
P.S.: The current patch does not even give an [early, intentional] error message in case I also think supporting |
comment:19
I still think removing The patch should IMHO at least test for P.S.: We could even attempt to download it (with e.g. |
This comment has been minimized.
This comment has been minimized.
comment:21
In addition to leif's comments, note that the pipestatus script says
This comment should probably be deleted. |
comment:22
Replying to @jhpalmieri:
Do you agree that this ticket needs works, w.r.t. the comment, and -- IMHO more important -- the lack of an error check + message on how to fix this? |
comment:23
I think the elephant in the room is #11073: remove the spkg/base repo, add a mechanism that adds the absolutely necessary files to the tarball (or directly downloads them for upgrades) without depending on bzip2/mercurial/.... Then |
This comment has been minimized.
This comment has been minimized.
comment:24
I'm attaching a referee patch printing an error message if diff --git a/sage-update b/sage-update
--- a/sage-update
+++ b/sage-update
@@ -400,6 +400,8 @@ def do_update():
os.chdir(SPKG_ROOT)
download_file("install")
os.system('chmod +x install')
+ download_file("pipestatus")
+ os.system('chmod +x pipestatus')
os.chdir(os.path.join(SPKG_ROOT, "standard"))
download_file("standard/deps")
download_file("standard/newest_version")
@@ -408,6 +410,7 @@ def do_update():
os.chdir(SAGE_ROOT)
if root_repo_intact:
for file in ['spkg/install',
+ 'spkg/pipestatus',
'spkg/standard/deps',
'spkg/standard/newest_version']:
subprocess.call('./sage -hg commit %s -m ' % file + but I'm not sure it's worth it: |
Attachment: trac_10970-error-msg.patch.gz apply on top of other patch |
comment:25
Replying to @vbraun:
Any results? Which version failed to upgrade? Concerning #11073: I don't see why it should affect this ticket. I think we should be able to merge this ticket now (if it doesn't break upgrades). |
Changed reviewer from Mariah Lenox to Jeroen Demeyer, Mariah Lenox |
comment:27
The only annoying thing here is that |
comment:28
|
Changed author from Volker Braun to none |
Changed reviewer from Jeroen Demeyer, Mariah Lenox to Jeroen Demeyer |
The
$SAGE_ROOT/spkg/install
has a here document that createspipestatus
. With the sage root repository, this is no longer needed aspipestatus
is tracked by the repository. The patch removes it from$SAGE_ROOT/spkg/install
.Patch moved to #11073.
CC: @jdemeyer @nexttime
Component: build
Keywords: sd32
Reviewer: Jeroen Demeyer
Issue created by migration from https://trac.sagemath.org/ticket/10970
The text was updated successfully, but these errors were encountered: