Fix sub-venv

[bertsky]

Fixes #140
Fixes #144

[bertsky]

703735d will also fix #151 (in the sense of offering a way out of stale/inconsistent state of ~/.parallel/semaphores; the original issue was duplicate of #140).

[bertsky]

@stweil @kba (as soon as CI shows green) can we merge this?

[stweil]

@stweil @kba (as soon as CI shows green) can we merge this?

This is again a pull request which mixes different fixes. That makes reviews and merging really difficult.

[kba]

Except for the venv deactivation thread, this is all resolved imho and can be merged.

[kba]

• Update my commits from PR Report missing sem instead of failing with error #153 to fix a misplaced argument and to fix Circle CI.

CI passes.

• Don't ignore failing commands. Some of the related changes are not needed, and for others it is better to improve the command than to ignore errors.

This has been resolved. ("Because not all module selections depend on IM. For example, if you exclude ocrd_olena, Docker will probably not have ImageMagick. (That Dockerfile is not only used for pre-built configs!)")

• Use the old chown variant for deps-ubuntu because portability it not an issue for that target and the old variant is shorter and better readable.

Done

• Please consider also my suggestions for the Dockerfile changes.

Done (see above)

So @stweil @bertsky anything missing or can we merge?

[bertsky]

anything missing or can we merge?

There's still a similar problem for wget

[bertsky]

Wow.

must replace which with command -v in Bourne sh

This is yet another portability nightmare. We apparently cannot rely on which, because some cache does not get updated soon enough after apt install. Also, they say which is actually from old csh, and has never been standardized. The proper way for Bourne shells or POSIX sh is said to be command -v, but bash in POSIX mode offers neither command nor type, which is stunning. That aspect is not even covered by this (otherwise exhaustive) SO thread on the subject.

Now what do we do? Crawl out and beg for SHELL = /usr/bin/env bash (using command -v), or find another tool, or debug the cache problem?

[bertsky]

This is yet another portability nightmare. We apparently cannot rely on which, because some cache does not get updated soon enough after apt install. Also, they say which is actually from old csh, and has never been standardized. The proper way for Bourne shells or POSIX sh is said to be command -v, but bash in POSIX mode offers neither command nor type, which is stunning. That aspect is not even covered by this (otherwise exhaustive) SO thread on the subject.

Now what do we do? Crawl out and beg for SHELL = /usr/bin/env bash (using command -v), or find another tool, or debug the cache problem?

Oh, got it: For one, /bin/sh is not bash on Debian, but dash. But more importantly, make itself (regardless of whether via shell function or recipe) will decide for itself not to invoke a shell, but search the executable in PATH, if it sees only one instruction. Thus, builtins like command don't work.

[stweil]

Thus, builtins like command don't work.

Well, command works for me without a hack, see this Makefile:

TEST := $(shell command -v make)
all:
	@echo test=$(TEST)

How did you get a different result? If it works for you, too, I suggest to remove all hacks again.

And there is no cache problem with which.

[stweil]

@bertsky, I now merged most of your commits, especially the urgent bug fixes. Please rebase your PR, so we can discuss the remaining ones easier.

[bertsky]

Well, command works for me without a hack, see this Makefile:

TEST := $(shell command -v make)
all:
	@echo test=$(TEST)

How did you get a different result?

You didn't say what your result was ;-)
With make version 4.1, I get:

make: command: Command not found
test=

But if I add a semicolon to the end of the shell call, then this becomes:

test=/usr/bin/make

Anyway, we don't need any which or command test, we can just $(shell PROG --version).

Meanwhile, I have also found the actual reason why our $(error ...) calls were all evaluated out of context: .EXPORT_ALL_VARIABLES breaks the usual expansion conditions of course. So removing this and using export VIRTUAL_ENV fixes all our wget/sem problems during deps-ubuntu.

It's even possible to run deps-ubuntu without having git before now (as the README already claimed).

[bertsky]

I now merged most of your commits, especially the urgent bug fixes. Please rebase your PR, so we can discuss the remaining ones easier.

What the ...

[bertsky]

I now merged most of your commits, especially the urgent bug fixes. Please rebase your PR, so we can discuss the remaining ones easier.

What the ...

@stweil you must be joking! You made me undo most of my PR after 3 days back and forth, and then just before I finally come around fixing the last issues, you just drag the commits out of the PR without even asking? I just reset master to the previous state, so we can wait for CI in the current, fixed state here and then merge if everyone is satisfied (as it should be).

[kba]

@bertsky, I now merged most of your commits, especially the urgent bug fixes. Please rebase your PR, so we can discuss the remaining ones easier.

@stweil why did you do that? We were close to finishing the discussions, weren't we?

[stweil]

why did you do that

I had the impression that it was time to fix some issues after a whole week of discussion. So I applied those patches where we agreed that they are fine.

What is the problem with this? Rebasing @bertsky's remaining commits is easy. I have already done in locally and could push that state to a new branch if that helps.

[kba]

why did you do that

I had the impression that it was time to fix some issues after a whole week of discussion. So I applied those patches where we agreed that they are fine.

We are all eager for a fix, but it should be based on consensus and successful CI. Cherry-picking individual commits and pushing them to master without consultation is just not good practice. We all have the power to push to master but we should only use it once we're in agreement. We were close but not yet there.

What is the problem with this? Rebasing @bertsky's remaining commits is easy. I have already done in locally and could push that state to a new branch if that helps.

Again, it's not a question of technicality but of courtesy. And you are - rightly - an adherent of concise pull requests and static code analysis. Cherry-picking from an almost finished PR with 90+ discussion threads without asking first here or in gitter is not good. I appreciate that the pace of discussions can sometimes be frustrating but finding consensus improves the product in the long-term.

[kba]

CI succeeded, the last commit only adapted the parallel citation in the README, which will not break building. So thanks a lot @bertsky and @stweil for the continued effort!

[bertsky]

What is the problem with this? Rebasing @bertsky's remaining commits is easy. I have already done in locally and could push that state to a new branch if that helps.

@stweil Also, you forget that rebasing then becomes much harder and a one-way direction. In this concrete case, I had already rebased this PR locally, fixing the early commits regarding the sem and wget failures (which you yourself have been pointing out mostly). We were in the middle of a discussion about these. By dragging them onto master directly, you made it impossible to edit/undo/reorder them. After all, that's what a PR is for: aggregating a set of mutually supporting/consistent commits that have to be considered and developed together until mature enough to integrate at once.