tcsh-org / tcsh

This is a read-only mirror of the tcsh code repository.
https://www.tcsh.org/
Other
229 stars 42 forks source link

[PATCH] Fix debian reproducibility test for tcsh #96

Closed josefs10 closed 4 months ago

josefs10 commented 4 months ago

tcsh 6.24.10-3 failed reproducibility tests in debian because of build variations with locale using the sort function. This patch switches LC_COLLATE with LC_ALL to fix that issue, based off this known reprotest issue. use-LC_ALL-with-sort.patch.txt

suominen commented 4 months ago

I did not find an explanation for the need to use LC_ALL instead of LC_COLLATE in the reprotest issue you linked to. Could you perhaps quote the relevant part in a comment here, please?

josefs10 commented 4 months ago

You are right, the bug description is not very good, but it showed me what to look at. So reprotest in the second build overrides LC_ALL when varying locale. Here is an example with some of the lines from tc.const.c that get sorted.

$ LANG=en_US.UTF-8 LANGUAGE=en_US.UTF-8:en LC_COLLATE=C sort ./file.txt
STR0
STR1
STRBB
STR_
STRaddsuffix
STRbang

$ LC_ALL=et_EE.UTF-8 LANGUAGE=et_EE.UTF-8:fr LC_COLLATE=C sort ./file.txt
STR_
STR0
STR1
STRaddsuffix
STRbang
STRBB

Because reprotest overrides LC_ALL, the output of the sort command varies which causes the changes in tc.const.h and the binary differences. Build log for failed reprotest is below. reprotest-fail-build.txt

suominen commented 4 months ago

LC_ALL is an override for all LC_* variables, whereas LANG is a default for them. I can't help but feel like this is a bug in reprotest: it's expectation that setting LC_ALL does not change anything seems incorrect to me.

Maybe we should just be unsetting LC_ALL, although we'd be doing it just to pacify reprotest. I'd rather have reprotest fixed.

Or is there some important logic behind the behaviour of reportest that I should understand?

josefs10 commented 4 months ago

I agree. Perhaps this is a reprotest issue (or reprotest feature that just causes problems here). This was my first time using reprotest, so I can talk to the maintainers to see if this can be fixed with some kind of override or if the default reprotest builds can be done without varying locale.

thep commented 4 months ago

LC_ALL is an override for all LC_* variables, whereas LANG is a default for them. I can't help but feel like this is a bug in reprotest: it's expectation that setting LC_ALL does not change anything seems incorrect to me.

I think I can understand why reprotest does this: its purpose is to simulate different build environments as much as possible, including when LC_ALL happens to be set, and the build should still reproduce the same result.

Maybe we should just be unsetting LC_ALL, although we'd be doing it just to pacify reprotest. I'd rather have reprotest fixed.

Unsetting LC_ALL is also fine, as long as all other locale categories have no effect on the output, which is the case here. I just think overriding LC_ALL here is simpler and safer.

I don't know the opinion of the reprotest maintainer, but I don't think it needs a fix for this.

suominen commented 4 months ago

@thep : I think you are right, it is safer to use LC_ALL, and now — having thought about it more — I think this is exactly the use case that LC_ALL was intended for.

Thank you both for taking the time to report the issue and to explain it to me. :)