Opened 4 years ago

Closed 4 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) 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 4 years ago by ncohen

  • Branch set to public/18513
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by git

  • Commit set to e8caa274577da75eadd532065cac08bd71eac316

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

e8caa27trac #18513: Make <package>/type file mandatory

comment:3 Changed 4 years ago by git

  • Commit changed from e8caa274577da75eadd532065cac08bd71eac316 to 066e88edb1a2278184f583406bb92aa2981d60f1

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

066e88etrac #18513: Make <package>/type file mandatory

comment:4 Changed 4 years ago by vbraun

lgtm but can we also have an error message?

comment:5 Changed 4 years ago by ncohen

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 4 years ago by vbraun

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: Changed 4 years ago by jdemeyer

  • Dependencies set to #17607
  • Status changed from needs_review to needs_work

Needs to be rebased over #17607.

comment:8 Changed 4 years ago by jdemeyer

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 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by ncohen

Needs to be rebased over #17607.

What the hell is this? You created a python2standard and python3standard package type?

comment:10 Changed 4 years ago by git

  • Commit changed from 066e88edb1a2278184f583406bb92aa2981d60f1 to 79a41b9cd4a69815eceafb8f14f190bb06e0fdd6

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

79a41b9trac #18513: Make <package>/type file mandatory

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

Replying to ncohen:

You created a python2standard and python3standard package type?

Well, I hope the You doesn't refer to me, since I am innocent.

comment:12 Changed 4 years ago by ncohen

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 in reply to: ↑ 11 Changed 4 years ago by ncohen

  • Status changed from needs_work to 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: Changed 4 years ago by jdemeyer

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: Changed 4 years ago by jdemeyer

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 4 years ago by git

  • Commit changed from 79a41b9cd4a69815eceafb8f14f190bb06e0fdd6 to e9054a633a2cad1adf61f409cc6023d3552f9aaa

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

e9054a6trac #18513: all -> none, and no output redirection

comment:17 in reply to: ↑ 14 Changed 4 years ago by ncohen

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:18 in reply to: ↑ 15 Changed 4 years ago by ncohen

Even simpler: you can replace

Sigh ....

comment:19 Changed 4 years ago by jdemeyer

  • Dependencies #17607 deleted

comment:20 Changed 4 years ago by git

  • Commit changed from e9054a633a2cad1adf61f409cc6023d3552f9aaa to a77b2226906dec5b8201155bd465bdf09b8021f8

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

a77b222trac #18513: simpler validity check

comment:21 Changed 4 years ago by jdemeyer

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

comment:22 Changed 4 years ago by vbraun

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