Opened 4 years ago

Closed 4 years ago

#18647 closed enhancement (fixed)

Automatically update new-style optional packages

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.8
Component: build Keywords:
Cc: vbraun, jdemeyer, jhpalmieri, kcrisman Merged in:
Authors: Nathann Cohen Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 68f4d0b (Commits) Commit: 68f4d0b138108f33e45452cdb3e7a67232cbac78
Dependencies: Stopgaps:

Description

With this branch, new-style installed optional packages which have not been installed to their latest available version are automatically updated during 'make'.

Nathann

Change History (43)

comment:1 Changed 4 years ago by ncohen

  • Branch set to public/18647
  • Commit set to 912753b28a7883185031381f43c12e032b9aa802
  • Status changed from new to needs_review

New commits:

912753btrac #18647: Automatically update new-style optional packages

comment:2 follow-up: Changed 4 years ago by jdemeyer

I would much prefer to add $(OPTIONAL_INSTALLED_PACKAGES) to all-sage instead of adding a new step in the all make rule.

comment:3 in reply to: ↑ 2 ; follow-ups: Changed 4 years ago by ncohen

I would much prefer to add $(OPTIONAL_INSTALLED_PACKAGES) to all-sage instead of adding a new step in the all make rule.

Then you must figure out which optional packages depend on which standard packages.

(it is not possible to add all standard packages as a dependency, as some 'optional' packages like python2 may also be standard in practice)

Last edited 4 years ago by ncohen (previous) (diff)

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

I would much prefer to add $(OPTIONAL_INSTALLED_PACKAGES) to all-sage instead of adding a new step in the all make rule.

Then you must figure out which optional packages depend on which standard packages.

If the goal is to have more reliable optional packages, this needs to be done anyway. If some optional package depends on PARI and PARI gets rebuilt, then you really should rebuild the optional package too.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by ncohen

If the goal is to have more reliable optional packages, this needs to be done anyway. If some optional package depends on PARI and PARI gets rebuilt, then you really should rebuild the optional package too.

Indeed, this is how it should have been implemented from the start. For you will agree that the problem already exists: if some optional package does not state its dependencies and if 'pari' (to take your example) is updated, the package is not re-built. At the moment, only gcc, scons, python2 and python3 state their dependencies.

Thus, and similarly to our recent debate on #18456, what you ask for is a very sensible bugfix for a pre-existing problem. You are welcome to address it -- and I will be glad to help -- but it is not a dependency of this ticket.

Nathann

comment:6 Changed 4 years ago by ncohen

By the way, fixing the dependencies can be done quite easily: by adding the necessary information in the build/<package/dependencies files you will get the result that you want. And we can merge the two rules together once we are sure that no dependency is missing.

comment:7 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

Thus, and similarly to our recent debate on #18456, what you ask for is a very sensible bugfix for a pre-existing problem.

But setting up the right framework such that dependencies can be considered is within the scope of this ticket.

Easy fix: set all dependencies of optional packages to | $(STANDARD_PACKAGES), which in practice is the same as your current patch, but allows to write the correct dependencies later.

comment:8 in reply to: ↑ 3 Changed 4 years ago by jdemeyer

Replying to ncohen:

it is not possible to add all standard packages as a dependency, as some 'optional' packages like python2 may also be standard in practice

Since many standard packages depend on Python, adding a dependency on $(STANDARD_PACKAGES) indirectly adds a dependency on Python.

comment:9 in reply to: ↑ 7 Changed 4 years ago by ncohen

Easy fix: set all dependencies of optional packages to | $(STANDARD_PACKAGES), which in practice is the same as your current patch, but allows to write the correct dependencies later.

Dead right. Will be done in a second.

comment:10 Changed 4 years ago by git

  • Commit changed from 912753b28a7883185031381f43c12e032b9aa802 to 287458d5b03be8b04dde32c6c904606e1ed951a6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

287458dtrac #18647: Automatically update new-style optional packages

comment:11 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Not mpir and gmp.

comment:12 Changed 4 years ago by ncohen

As a consequence: all optional packages have (and *MUST* have) a /dependencies file.

Should we check this somewhere?

comment:13 Changed 4 years ago by git

  • Commit changed from 287458d5b03be8b04dde32c6c904606e1ed951a6 to cca953e2a0cc3724877e845853df51efc47f2a24

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cca953etrac #18647: Automatically update new-style optional packages

comment:14 follow-up: Changed 4 years ago by ncohen

Right. Fixed. Cursed be those non-optional optional packages.

Last edited 4 years ago by ncohen (previous) (diff)

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

Right. Fixed. Cursed be those non-optional optional packages.

Well, I proposed a new category for these packages :-)

I also wonder if we can use

if [ -f $SAGE_SPKG_INST/$PKG_NAME-* ]

since that will not work if there is more than 1 matching file.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by ncohen

Well, I proposed a new category for these packages :-)

Oh. The alternative guys?

I also wonder if we can use

if [ -f $SAGE_SPKG_INST/$PKG_NAME-* ]

since that will not work if there is more than 1 matching file.

I think that's okay, for you made it quite clear that a version number must be split from the name by a '-'? I don't mind adding code that checks that somewhere if you think it necessary, but renaming the packages that do not respect this may take quite some time.

Nathann

Last edited 4 years ago by ncohen (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 4 years ago by jdemeyer

Replying to ncohen:

I think that's okay

Right, I agree.

comment:18 Changed 4 years ago by jdemeyer

I don't think this works correctly for gcc. If gcc needs to be reinstalled, it should be added to $(TOOLCHAIN). The problem is that installing gcc while installing a different package will eventually lead to trouble.

comment:19 Changed 4 years ago by jdemeyer

For gcc, we already have logic in build/install to decide whether it should be (re-)installed. You shouldn't interfere with that.

comment:20 follow-up: Changed 4 years ago by ncohen

So I filter it out?

comment:21 in reply to: ↑ 20 Changed 4 years ago by jdemeyer

Replying to ncohen:

So I filter it out?

Or change the type to something else (if you don't want to add a new type, it must be base)

comment:22 follow-up: Changed 4 years ago by ncohen

HMmmmmmmm... If I rename it to 'base' it will be automatically installed. I'll just filter it out. Can you tell me what exactly is wrong if we leave it as it is? It will only force an update if it has already been installed, and if the version changes. It looks harmless?...

Nathann

comment:23 in reply to: ↑ 22 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

HMmmmmmmm... If I rename it to 'base' it will be automatically installed.

Why? Since when are we automatically installing base packages?

Can you tell me what exactly is wrong if we leave it as it is? It will only force an update if it has already been installed, and if the version changes. It looks harmless?...

Read the last sentence of 18

comment:24 in reply to: ↑ 23 ; follow-up: Changed 4 years ago by ncohen

Why? Since when are we automatically installing base packages?

Well, not automtically.. There is a 'base' rule in the makefile, but the dependencies are set manually. Probably because of gcc actually.

Read the last sentence of 18

Okay, so tha's because gcc is gcc. I'll filter it out.

Nathann

comment:25 Changed 4 years ago by git

  • Commit changed from cca953e2a0cc3724877e845853df51efc47f2a24 to 4b55fc4e97952a83983c2015674462bb2f593db4

Branch pushed to git repo; I updated commit sha1. New commits:

4b55fc4trac #18647: Filter gcc out

comment:26 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:27 follow-up: Changed 4 years ago by jhpalmieri

I know you've done all of this work to create these files now, but if a package is optional and doesn't have a dependencies file, shouldn't we automatically set the dependencies to | $(STANDARD_PACKAGES)? That seems cleaner to me.

comment:28 in reply to: ↑ 27 Changed 4 years ago by ncohen

I know you've done all of this work to create these files now, but if a package is optional and doesn't have a dependencies file, shouldn't we automatically set the dependencies to | $(STANDARD_PACKAGES)? That seems cleaner to me.

Well... There would be less things in the files for sure, but a new rule which is rather hard to figure out by yourself. Honestly I don't know... Jeroen?

comment:29 follow-up: Changed 4 years ago by ncohen

If a guy notices that his package depends on another package and wants to add the dependency, suddenly he would have to create the file and add the implicit dependency | $(STANDARD_PACKAGES).

comment:30 Changed 4 years ago by ncohen

Really I do not know. I don't mind implementing it if you are both convinced that it is a good idea, but really I don't know.

comment:31 in reply to: ↑ 29 Changed 4 years ago by jdemeyer

Replying to ncohen:

If a guy notices that his package depends on another package and wants to add the dependency, suddenly he would have to create the file and add the implicit dependency | $(STANDARD_PACKAGES).

No, the dependency | $(STANDARD_PACKAGES) is not a real dependency, it's just because we are too lazy to add the correct dependencies.

So, I think John's proposal might be good (but be sure to keep the empty dependencies for mpir and gmp)

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:32 Changed 4 years ago by ncohen

Okayokay, no problem then. I'll do that tomorrow morning (sorry guys, I really need to sleep ^^;)

comment:33 Changed 4 years ago by git

  • Commit changed from 4b55fc4e97952a83983c2015674462bb2f593db4 to 3379d0e4e354c67dcc9d21c47e1322fe6e00cd1c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3379d0etrac #18647: Automatically update new-style optional packages

comment:34 Changed 4 years ago by ncohen

Here it is. I rewrote history, so that one commit does not undo what the previous does.

Nathann

comment:35 Changed 4 years ago by git

  • Commit changed from 3379d0e4e354c67dcc9d21c47e1322fe6e00cd1c to 83759dbbf5c3d04377ad98551de1146cb0dfd106

Branch pushed to git repo; I updated commit sha1. New commits:

83759dbtrac #18647: no dependencies for mpir and gmp

comment:36 Changed 4 years ago by git

  • Commit changed from 83759dbbf5c3d04377ad98551de1146cb0dfd106 to 4a744884a9815926ce2d9c6a9b38ee72cfc3de40

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4a74488trac #18647: no dependencies for mpir and gmp

comment:37 Changed 4 years ago by jdemeyer

Replace

[ "x$(cat $TYPE_FILE)" = "xoptional" ]

by

[ x`cat "$TYPE_FILE"` = xoptional ]

(the point is: you need to quote $TYPE_FILE)

comment:38 Changed 4 years ago by jdemeyer

And "| \$(STANDARD_PACKAGES)" can be '| $(STANDARD_PACKAGES)' (avoid ugly backslashes if you can)

comment:39 Changed 4 years ago by git

  • Commit changed from 4a744884a9815926ce2d9c6a9b38ee72cfc3de40 to 68f4d0b138108f33e45452cdb3e7a67232cbac78

Branch pushed to git repo; I updated commit sha1. New commits:

68f4d0btrac #18647: Reviewer's comments

comment:40 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:41 Changed 4 years ago by ncohen

Excellent news! Thanks ;-)

comment:42 in reply to: ↑ 24 Changed 4 years ago by jdemeyer

Replying to ncohen:

Why? Since when are we automatically installing base packages?

Well, not automtically.. There is a 'base' rule in the makefile, but the dependencies are set manually.

Don't confuse the base make rule with base package(s) (maybe that's actually a good reason to rename the base packages to something else).

comment:43 Changed 4 years ago by vbraun

  • Branch changed from public/18647 to 68f4d0b138108f33e45452cdb3e7a67232cbac78
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.