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

dependencies: use "foo" instead of "$(INST)/$(FOO)" #20140

Closed
jdemeyer opened this issue Feb 29, 2016 · 37 comments
Closed

dependencies: use "foo" instead of "$(INST)/$(FOO)" #20140

jdemeyer opened this issue Feb 29, 2016 · 37 comments

Comments

@jdemeyer
Copy link

Replace dependencies like $(INST)/$(PARI) by pari.

configure tarball (for buildbot testing): http://sage.ugent.be/www/jdemeyer/sage/configure-147.tar.gz

Depends on #20130

CC: @simon-king-jena @vbraun @kiwifb

Component: build

Author: Jeroen Demeyer

Branch/Commit: 383b1b4

Reviewer: Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/20140

@jdemeyer jdemeyer added this to the sage-7.1 milestone Feb 29, 2016
@jdemeyer
Copy link
Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

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

bbb688adependencies: use "foo" instead of "$(INST)/$(FOO)"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

Commit: bbb688a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

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

9145390dependencies: use "foo" instead of "$(INST)/$(FOO)"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

Changed commit from bbb688a to 9145390

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

Changed commit from 9145390 to f26772f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

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

f26772fdependencies: use "foo" instead of "$(INST)/$(FOO)"

@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

Changed commit from f26772f to 87d5afd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

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

87d5afddependencies: use "foo" instead of "$(INST)/$(FOO)"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

Changed commit from 87d5afd to 463d22a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2016

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

463d22adependencies: use "foo" instead of "$(INST)/$(FOO)"

@jdemeyer

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Mar 1, 2016

comment:9

Ah, I was just thinking about this. Actually I had a broader proposal that I'd expand on later of cobbling all package metadata into a single machine-consumable file that I would like to use to better generate info on packages. But just improving the format of the "dependencies" file is a good start.

@kiwifb
Copy link
Member

kiwifb commented Mar 1, 2016

comment:11

I don't usually dwell in that area. It made me read some documentation. It does look ok and is a bit simpler.

@vbraun
Copy link
Member

vbraun commented Mar 1, 2016

comment:12

Lgtm but can you also merge in #20129

@jdemeyer
Copy link
Author

jdemeyer commented Mar 1, 2016

comment:13

Replying to @vbraun:

Lgtm but can you also merge in #20129

Why? There is no conflict (openblas has no dependencies).

@vbraun
Copy link
Member

vbraun commented Mar 1, 2016

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Mar 1, 2016

comment:14

Right, forgot about that.

@vbraun
Copy link
Member

vbraun commented Mar 1, 2016

comment:15

Conflicts with #20130

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2016

Changed commit from 463d22a to d7e720f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2016

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

18d59bfMove third-party packages to pkg-config blas
de7efd2Fix typo: use lapack.pc not blas.pc
f1302b2Bump patchlevels
555b258Iml version/patchlevel fix
d7e720fMerge commit '555b25811afe8da5d83c3ed6461e423a70f961f7' into t/20140/dependencies__use__foo__instead_of____inst____foo__

@jdemeyer
Copy link
Author

jdemeyer commented Mar 2, 2016

Dependencies: #20130

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2016

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2e1cbc5Use sed to transform dependencies from "foo" to "$(inst_foo)"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2016

Changed commit from d7e720f to 2e1cbc5

@jdemeyer
Copy link
Author

jdemeyer commented Mar 2, 2016

comment:19

Previous version didn't work because of phony targets in the dependencies. This new version still allows dependencies to be listed as foo. A small sed script transforms this to $(inst_foo).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2016

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

383b1b4Use sed to transform dependencies from "foo" to "$(inst_foo)"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2016

Changed commit from 2e1cbc5 to 383b1b4

@embray
Copy link
Contributor

embray commented Mar 2, 2016

comment:21

While you're at it, I've noticed that some packages don't have all their dependencies listed. I don't have an exhaustive list yet, but I just encountered that jsonschema is only included as a dependency of nbformat even though it's not listed as such.

Another one: mistune is a dependency of nbconvert.

@vbraun
Copy link
Member

vbraun commented Mar 2, 2016

comment:22

If we had a way of getting the dependency information into Python then it would be easy enough to compare with the pip dependency information for testing....

@jdemeyer
Copy link
Author

jdemeyer commented Mar 2, 2016

comment:23

Replying to @embray:

jsonschema is only included as a dependency of nbformat even though it's not listed as such.

I don't understand this sentence

@embray
Copy link
Contributor

embray commented Mar 2, 2016

comment:24

The only reason jsonschema has an spkg is sage is because it's a dependency of nbformat, but nbformat does not list jsonschema in its dependencies.

@jdemeyer
Copy link
Author

jdemeyer commented Mar 2, 2016

comment:25

Maybe you are confusing run-time dependencies with build-time dependencies? The dependencies files are really only meant for build-time dependencies.

Anyway, this isn't what this ticket is about...

@embray
Copy link
Contributor

embray commented Mar 2, 2016

comment:26

Okay, that must be it then. I'm confused because some of the Python packages do list dependencies that I wouldn't think of as build dependencies (unless maybe the package doesn't import without them).

@jdemeyer
Copy link
Author

jdemeyer commented Mar 2, 2016

comment:27

Replying to @embray:

I'm confused because some of the Python packages do list dependencies that I wouldn't think of as build dependencies (unless maybe the package doesn't import without them).

First of all, keep in mind that a test-suite is also considered "build-time". But it could very well be that packages list dependencies which are not strictly required. Usually, the time to build Python packages is negligible, so it's not so important.

@jdemeyer
Copy link
Author

jdemeyer commented Mar 2, 2016

comment:28

Regardless of all this, can somebody review this ticket please?

@vbraun
Copy link
Member

vbraun commented Mar 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants