Make <package>/type file mandatory

[ncohen]

With this branch, "make" is interrupted if some package does not define a 'type' file, or if its content is unexpected.

Nathann

[git]

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

​e8caa27

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

[git]

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

​066e88e

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

[vbraun]

lgtm but can we also have an error message?

[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

[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"?

[jdemeyer]

Needs to be rebased over #17607.

[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.

[ncohen]

Needs to be rebased over #17607.

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

[git]

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

​79a41b9

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

[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.

[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

[ncohen]

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

[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.

[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 $?

[git]

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

​e9054a6

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

[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

[ncohen]

Even simpler: you can replace

Sigh ....

[git]

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

​a77b222

trac #18513: simpler validity check