Opened 8 years ago
Closed 8 years ago
#18513 closed enhancement (fixed)
Make <package>/type file mandatory
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | build | Keywords: | |
Cc: | jdemeyer, vbraun | Merged in: | |
Authors: | Nathann Cohen | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | a77b222 (Commits, GitHub, GitLab) | Commit: | a77b2226906dec5b8201155bd465bdf09b8021f8 |
Dependencies: | Stopgaps: |
Description
With this branch, "make" is interrupted if some package does not define a 'type' file, or if its content is unexpected.
Nathann
Change History (22)
comment:1 Changed 8 years ago by
Branch: | → public/18513 |
---|---|
Status: | new → needs_review |
comment:2 Changed 8 years ago by
Commit: | → e8caa274577da75eadd532065cac08bd71eac316 |
---|
comment:3 Changed 8 years ago by
Commit: | e8caa274577da75eadd532065cac08bd71eac316 → 066e88edb1a2278184f583406bb92aa2981d60f1 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
066e88e | trac #18513: Make <package>/type file mandatory
|
comment:5 Changed 8 years ago by
Well, there is one generated by 'cat' when the file does not exist, and it prints something specific about what the content of the 'type' file should be when it is invalid.
Do you want something 'hand-made' when the file does not exist?
Nathann
comment:6 Changed 8 years ago by
If I just see cat erroring out then it looks like a bug in the build system. And you catch the error anyways, so why not print something like "the package type must declare its 'type' file"?
comment:7 follow-up: 9 Changed 8 years ago by
Dependencies: | → #17607 |
---|---|
Status: | needs_review → needs_work |
Needs to be rebased over #17607.
comment:8 Changed 8 years ago by
I actually think that this is pretty clear, it explicitly says which file is missing:
$ rm build/pkgs/r/type # Just for testing $ make cd build && \ "../build/pipestatus" \ "env SAGE_PARALLEL_SPKG_BUILD='' ./install all 2>&1" \ "tee -a ../logs/install.log" cat: /usr/local/src/sage-config/build/pkgs/r/type: No such file or directory Makefile:19: recipe for target 'build' failed make: *** [build] Error 1
However, to reduce confusion, it doesn't hurt to add an extra error message.
comment:9 follow-up: 11 Changed 8 years ago by
Needs to be rebased over #17607.
What the hell is this? You created a python2standard
and python3standard
package type?
comment:10 Changed 8 years ago by
Commit: | 066e88edb1a2278184f583406bb92aa2981d60f1 → 79a41b9cd4a69815eceafb8f14f190bb06e0fdd6 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
79a41b9 | trac #18513: Make <package>/type file mandatory
|
comment:11 follow-up: 13 Changed 8 years ago by
Replying to ncohen:
You created a
python2standard
andpython3standard
package type?
Well, I hope the You doesn't refer to me, since I am innocent.
comment:12 Changed 8 years ago by
There does not seem to be a conflict with #17607. But I find what was done there *VERY* dirty. A 'clean' way out would have been to 1) Create a 'standard' virtual 'Python' package with dependency on 'python_2_or_3' 2) Make python2 and python3 optional 3) Create in build/install a rule 'python_2_or_3' depending on python2 or python3.
There would have been no need of adding two new types, and no need to change filtered_packages_list.
Nathann
comment:13 Changed 8 years ago by
Status: | needs_work → needs_review |
---|
Well, I hope the You doesn't refer to me, since I am innocent.
So you are innocent.
I set this ticket back to needs_review, as there is apaprently no conflict with #17607.
Nathann
comment:14 follow-up: 17 Changed 8 years ago by
Can you replace
filtered_packages_list all > /dev/null
by
filtered_packages_list none
because of efficiency reasons? With the first variant, you are executing a lot of stuff which you don't use.
comment:15 follow-up: 18 Changed 8 years ago by
Even simpler: you can replace
filtered_packages_list all > /dev/null if [ $? -ne 0 ]; then exit 1 fi
by
filtered_packages_list none || exit $?
comment:16 Changed 8 years ago by
Commit: | 79a41b9cd4a69815eceafb8f14f190bb06e0fdd6 → e9054a633a2cad1adf61f409cc6023d3552f9aaa |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
e9054a6 | trac #18513: all -> none, and no output redirection
|
comment:17 Changed 8 years ago by
With the first variant, you are executing a lot of stuff which you don't use.
Done. In my first version of that function an error was raised when the argument was anything which is not a valid type.
Nathann
comment:19 Changed 8 years ago by
Dependencies: | #17607 |
---|
comment:20 Changed 8 years ago by
Commit: | e9054a633a2cad1adf61f409cc6023d3552f9aaa → a77b2226906dec5b8201155bd465bdf09b8021f8 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a77b222 | trac #18513: simpler validity check
|
comment:21 Changed 8 years ago by
Reviewers: | → Jeroen Demeyer |
---|---|
Status: | needs_review → positive_review |
comment:22 Changed 8 years ago by
Branch: | public/18513 → a77b2226906dec5b8201155bd465bdf09b8021f8 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Branch pushed to git repo; I updated commit sha1. New commits:
trac #18513: Make <package>/type file mandatory