New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use sort which is independent of current locale #898

Merged
merged 1 commit into from Nov 8, 2016

Conversation

Projects
None yet
3 participants
@hannesm
Member

hannesm commented Nov 8, 2016

According to POSIX, sort is local-dependent. This leads to an actual problem here, since ​caml_sys_is_directory and ​caml_s​ys_isatty are reordered depending on locale.

POSIX says: "When using sort to process pathnames, it is recommended that LC_ALL, or at least LC_CTYPE and LC_COLLATE, are set to POSIX or C in the environment, since pathnames can contain byte sequences that do not form valid characters in some locales, in which case the utility's behavior would be undefined. In the POSIX locale each byte is a valid single-byte character, and therefore this problem is avoided."

This was found via the reproducible automated build - which atm has another issue that the build path is put (over and over) in binaries (..a/.o/.so/.cma/.cmt/.cmti/.cmxs), which should be addressed at another point - see diffoscope OCaml-4.03 (or download the full 140MB diff if you like) for details (or some compiler output, e.g. findlib). For the full set of differences between reproducible runs, look here.

I have no access to MS Windows (neither MacOSX) equipment

Credits for @infinity0 and @gasche for poking me investigating and submitting a PR here.

@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Nov 8, 2016

Contributor

As a further note, the vast majority of the build-path differences is due to GCC and hopefully should be fixed by my patch here. If/after that is accepted, the diff should become a bit more manageable. In the meantime this snippet does a half-decent job of filtering them out:

$ grep '│ [-+]' ocaml_4.03.0-5.diffoscope.txt | grep -v 'build-1st\|build-2nd\|DW_AT_\|\b\.\?debug\b'
Contributor

infinity0 commented Nov 8, 2016

As a further note, the vast majority of the build-path differences is due to GCC and hopefully should be fixed by my patch here. If/after that is accepted, the diff should become a bit more manageable. In the meantime this snippet does a half-decent job of filtering them out:

$ grep '│ [-+]' ocaml_4.03.0-5.diffoscope.txt | grep -v 'build-1st\|build-2nd\|DW_AT_\|\b\.\?debug\b'
@infinity0

This comment has been minimized.

Show comment
Hide comment
@infinity0

infinity0 Nov 8, 2016

Contributor

Oh, not "vast majority" - my snippet was too eager. There is also quite a lot of differences in cmxs, cmt files etc.

$ grep '│ [-+]' ocaml_4.03.0-5.diffoscope.txt | grep -v 'DW_AT_\|\b\.\?debug\b' | less

^ that should be a bit more targeted towards ignoring only the GCC things.

Contributor

infinity0 commented Nov 8, 2016

Oh, not "vast majority" - my snippet was too eager. There is also quite a lot of differences in cmxs, cmt files etc.

$ grep '│ [-+]' ocaml_4.03.0-5.diffoscope.txt | grep -v 'DW_AT_\|\b\.\?debug\b' | less

^ that should be a bit more targeted towards ignoring only the GCC things.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 8, 2016

Member

I pushed this change to INRIA's continuous integration machines, that have checked it on all sort of MacOS and Windows machines (both msvc and cygwin), and it seems to build fine there. I would be in favor of merging the PR.

The one thing I regret is that you don't have a change entry. I agree the change is very small, and not directly noticeably by users, but I think the larger arc of reproducible-build work is interesting, also work-consuming, so you and @infinity0 would deserve to get explicit credit for it. I would suggest the following:

### Build system:

- GPR#898: fix locale-dependence of primitive list order,
  detected through reproducible-builds.org.
  (Hannes Mehnert, review by Gabriel Scherer and Ximin Luo)

(The "Build system" category was present in the 4.04.0 Changelog, but it's not present in 4.05 changes yet.)

Member

gasche commented Nov 8, 2016

I pushed this change to INRIA's continuous integration machines, that have checked it on all sort of MacOS and Windows machines (both msvc and cygwin), and it seems to build fine there. I would be in favor of merging the PR.

The one thing I regret is that you don't have a change entry. I agree the change is very small, and not directly noticeably by users, but I think the larger arc of reproducible-build work is interesting, also work-consuming, so you and @infinity0 would deserve to get explicit credit for it. I would suggest the following:

### Build system:

- GPR#898: fix locale-dependence of primitive list order,
  detected through reproducible-builds.org.
  (Hannes Mehnert, review by Gabriel Scherer and Ximin Luo)

(The "Build system" category was present in the 4.04.0 Changelog, but it's not present in 4.05 changes yet.)

@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Nov 8, 2016

Member

@gasche thx; sounds good to me. will you add the changes entry when merging, or should I copy+paste your text and commit here?

Member

hannesm commented Nov 8, 2016

@gasche thx; sounds good to me. will you add the changes entry when merging, or should I copy+paste your text and commit here?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 8, 2016

Member

I think the best is for your to include a change entry in your commit, amend, and push-force, so that it comes with the modification itself instead of separately.

Member

gasche commented Nov 8, 2016

I think the best is for your to include a change entry in your commit, amend, and push-force, so that it comes with the modification itself instead of separately.

@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Nov 8, 2016

Member

done, there was a ### Compiler distribution build system: which sounded like the right heading to me

Member

hannesm commented Nov 8, 2016

done, there was a ### Compiler distribution build system: which sounded like the right heading to me

@gasche gasche merged commit e4ec5ee into ocaml:trunk Nov 8, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 8, 2016

Member

Thanks! Indeed.

Member

gasche commented Nov 8, 2016

Thanks! Indeed.

@hannesm hannesm deleted the hannesm:stable-sort branch Nov 8, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #898 from hannesm/stable-sort
use sort which is independent of current locale
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment