-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
switch sage to the new directory layout #14480
Comments
comment:1
This can really only be reviewed on the new trac instance. For now, the branch field on http://trac.tangentspace.org/ticket/13015 links to the list of changes (to actually see the changes click the view changes button -- since this includes some massive changes, it might take awhile to load the side-by-side diff). |
comment:5
Where is the script that you used to automatically create the git repo from the hg repos? That should be reviewed too and kept for reference. |
This comment has been minimized.
This comment has been minimized.
Branch: u/ohanar/build_system |
comment:9
A few small issues with this branch. I am not sure which of these should actually be fixed here, so I'm marking the ticket as
|
comment:10
Thanks for having a look. Replying to @mezzarobba:
This is now #14954.
This is fixed by #14330. I believe that these issues are not related to |
comment:11
These are the changes that actually need review for this ticket:
As a diff (this usually times out on the trac server): As a list of commits: |
comment:12
The following is my review for the changes between master and Overall, I'm impressed by the amount of work this has been. Maybe it makes sense for at least one more person to go through this diff.
why do we clean more directories than before?
What happened to this parallel_make.cfg file? Do I understand correctly that, and is it okay that the micro_release code now
There's still a reference to the spkg directory here.
What did this script do? Why don't we need it anymore?
Should we make sure that this does not break any optional/experimental packages?
This reminds me: will we, at some point, test the installation of the
This change has been made in quite some places. Why wasn't GREP_OPTIONS being set
It feels wrong to me to prefer the src/bin version of the sage script to
Why doesn't this need sage location anymore? @@ -904,25 +861,7 @@ fi
Why don't we call sage-location anymore? and sage-starts?
We need to set this to the trac server. Also, what branch does this clone? It
This looks fine, but why wasn't it an issue before?
This can probably go.
This should be set to the trac server.
This file has had major modifications so it needs updated author credits. Also,
I think this is an odd way to change the functionality. Maybe download-only
Why does sage-sync-build need so many changes? Naively, it would need only a few
Maybe do a
Will this still work on Cygwin?
why did this change? Is this related to the removal of whitespace? If so:
Do I understand correctly that we install sqlalchemy when running doctests? What is
Why don't we need this anymore? |
comment:13
Replying to @tkluck:
Definitely. And hopefully someone who is already familiar with Sage's build system.
In order to make sure distclean still works as expected. Previously cleaning up everything wasn't important for this since distclean would just delete the whole devel directory.
There is no other reference to it in the source code.
It seems like I didn't revert some cleanup I made to the Makefile in response to #14721.
it is used to generate the webpages http://sagemath.org/packages/{standard,optional,experimental}, however the actual version of the script that is used in generating these patches disagrees from the version that is included in Sage. Since I was overhauling the build system (and this was in the directory related to the build system), I thought it wise to remove it to avoid any confusion.
We should at least make sure that all optional packages work. Basically all changes that might be needed should be fixable within the mercurial distribution. Since we will have to through the packages anyway, it made perfect sense to me to cleanup the whole SAGE_DATA thing. I've opened #14962 for this task.
It was, however I didn't bother fixing it on the mercurial side because either you have to unset it in sage-env or you have to do it in a bunch of packages (although it just came to me that you could unset it in sage-spkg). Unsetting in sage-env is less than ideal because it means that commands like sage -grep don't respect GREP_OPTIONS, and changing a simple thing like this across a bunch of packages is incredibly painful in the current workflow.
I've gone back and forth on this a few times. The reason why I ended up with this was because the latest version of the sage script will always be at src/bin, where as the version at local/bin will only be updated if you have rerun make.
This was a merging error, sage-location should still be here.
I moved the sage-location call to sage-real-upgrade, and I think that sage-starts should probably be moved to sage-real-upgrade as well (which I think I meant to do, but it seems like I forgot).
Without an argument git clone will clone HEAD.
It was, but it is more of an issue now because sage-spkg now tries a few urls for each package, which requires sage-download-file to bail if the server returns an error.
Currently we are symlinking stuff into SAGE_LOCAL from the devel directory (this is to make cloning fast), however this is no longer happening in the git repository. This caused some silly breaking with the sage-sync-build, which was in need of some heavy revision anyway. There is now another ticket out there (don't remember which one) dedicated to fixing + updating this script.
Good idea.
Pushing should only be done by the release manager. I think the best place for such stuff would be as part of the development scripts, although I think adding the release manager portion to these scripts should be lower priority than the general developer portion of these scripts.
I think so, although we really should have someone on Cygwin test. As I mentioned, the symlinks that the comment refers to are no more.
This can be changed in sage-spkg. That message just prints when it realizes it has the package metadata locally.
no other reference in the source code (except for MANIFEST.in, which should be updated) |
comment:14
Mind if I start making some of the (simpler) changes?
I couldn't find the ticket. Do you mean that your edits basically fix the ticket? Or do you mean that the version of
I agree that this is the behavior, but I'm not sure whether that's what we want. After switching branches, you could end up in the situation where the But I guess that I don't mind much; whoever is in a situation where the difference affects them, is probably doing something with the internal workings of sage anyway, and should be able to deal with it. |
comment:15
I think it makes sense to move |
comment:16
Ok, I think I addressed Timo's comments, with the following exceptions: HG configuration in sage-env: I don't see a good reason for getting rid of these in the short term, we may not be using hg anymore, but it could still be helpful for the transition period while repositories there are still HG repositories for optional packages. GREP_OPTIONS: I'm not going to fix this now, instead leave it to a later date (unless I encounter merge conflicts, which so far I haven't). Replying to @sagetrac-felixs:
I would rather not, I don't see a huge need for change at this moment and currently everything works. We can always change this later down the road, but for now I would like to get this ticket finished. One other thing is that some functionality (especially sdist and bdist) might be broken until I merge the git spkg, which I will do in this ticket. However, there is still plenty of other code that needs review so I'm going to mark this to needs review for the moment. |
comment:18
ok, merged in the git spkg, so all should be good to go |
Branch pushed to git repo; I updated commit sha1. New commits: |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:47
Looks good to me! |
Reviewer: Volker Braun |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits: |
comment:49
For the record, this ticket is positively reviewed. Even if Andrew keeps merging the sage-hg branch until sage-5.13 is finished. |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits: |
comment:51
Hi, The merge of |
Branch pushed to git repo; I updated commit sha1. New commits: |
Branch pushed to git repo; I updated commit sha1. New commits: |
To switch to a unified repository we need to include some information that are currently in the spkg tarballs, which requires modifying the sage's directory structure. Since we are switching workflows at the same time, it is an ideal time to rethink the directory structure as a whole.
The proposed significant changes
Full changes are described at http://wiki.sagemath.org/WorkflowSEP.
The script used to create the git repository is
consolidate-repos.sh
, which depends onconfiguration.sh
,git-filter-branch
, and thefast-export
directory (all located at https://github.com/sagemath/sage-workflow). They require bash4, git 1.8.x, and python 2.7 to run (at the very least, possibly more).Depends on #13015
Depends on #14781
CC: @jdemeyer @saraedum
Component: distribution
Author: R. Andrew Ohana
Branch/Commit: u/ohanar/build_system @
cd237e7
Reviewer: Volker Braun
Issue created by migration from https://trac.sagemath.org/ticket/14480
The text was updated successfully, but these errors were encountered: