Filename: Do not raise an assertion failure on module load#674
Merged
mshinwell merged 1 commit intoocaml:trunkfrom Jul 8, 2016
Merged
Filename: Do not raise an assertion failure on module load#674mshinwell merged 1 commit intoocaml:trunkfrom
mshinwell merged 1 commit intoocaml:trunkfrom
Conversation
If `Sys.os_type` is set to an unknown value (e.g. `xen` in the case of MirageOS), the Filename module currently raises an assertion failure at initialisation time. This patch alters Filename to default to Unix semantics if the `os_type` is unknown, therefore letting more progress be made. It is quite unlikely that `Filename` will ever actually be used in these embedded environments, but it is quite frequently linked in due to being a stdlib dependency and being referencd from something. See also mirage/mirage#123
Contributor
|
Seems reasonable to me. I will merge soon if nobody is against. |
This was referenced Nov 7, 2016
Member
Author
|
@gasche: looks like this got missed from the Changes file? |
Member
|
Indeed, sorry. Generally we expect commits to come with their own Changes entry, but it seems that the people that handled this PR forgot to ask about it. (On some rare occasions it is fine not to have a Changes entry.) A while ago I added a travis-ci test to check that people write a Changes entry, but it is marked as an optional test, and Travis' interface does not let anyone know in an effective way that this check failed, unless you explicitly look for it (see the travis page for this PR). Maybe we should actually mark it mandatory, to always notice when it does not pass, but still accept no-Changes cases on an exceptional basis. |
Member
gasche
added a commit
to gasche/ocaml
that referenced
this pull request
Nov 12, 2016
Travis CI (Continuous Integration) tests provide feedback to people contributing patches and pull requests. There has long been a test checking that the Changes file is modified by the PR (along with a test checking that the testsuite is modified), but it was marked optional (`allow_failures`) and not reported in a clear way at all. (In theory all user-visible or contributor-visible changes should get a Changes entry. There are some cases of minor changes, or changes that affect features not present in a released version, where not having a dedicated changelog entry is ok.) There are two cases where forgetting to include something in the Changes has been problematic: - inline records were originally not part of the 4.03 Changelog (we caught the error as I gave a talk on new 4.03 features and Alain gently asked why I left inline records out of the discussion) - recently GPR ocaml#674 was forgotten from the Changelog If we make the Change mandatory in the CI, it is because without it there is no clear notification than it fails. It does not mean that having a Changelog entry is now mandatory, and it's just fine to keep deciding on a case-by-case basis not to have an entry. It's just that we would make that choice voluntarily, instead of out of distraction.
gasche
added a commit
to gasche/ocaml
that referenced
this pull request
Nov 12, 2016
Travis CI (Continuous Integration) tests provide feedback to people contributing patches and pull requests. There has long been a test checking that the Changes file is modified by the PR (along with a test checking that the testsuite is modified), but it was marked optional (`allow_failures`) and not reported in a clear way at all. (In theory all user-visible or contributor-visible changes should get a Changes entry. There are some cases of minor changes, or changes that affect features not present in a released version, where not having a dedicated changelog entry is ok.) There are two cases where forgetting to include something in the Changes has been problematic: - inline records were originally not part of the 4.03 Changelog (we caught the error as I gave a talk on new 4.03 features and Alain gently asked why I left inline records out of the discussion) - recently GPR ocaml#674 was forgotten from the Changelog If we make the Change mandatory in the CI, it is because without it there is no clear notification than it fails. It does not mean that having a Changelog entry is now mandatory, and it's just fine to keep deciding on a case-by-case basis not to have an entry. It's just that we would make that choice voluntarily, instead of out of distraction.
camlspotter
pushed a commit
to camlspotter/ocaml
that referenced
this pull request
Oct 17, 2017
camlspotter
pushed a commit
to camlspotter/ocaml
that referenced
this pull request
Oct 17, 2017
Travis CI (Continuous Integration) tests provide feedback to people contributing patches and pull requests. There has long been a test checking that the Changes file is modified by the PR (along with a test checking that the testsuite is modified), but it was marked optional (`allow_failures`) and not reported in a clear way at all. (In theory all user-visible or contributor-visible changes should get a Changes entry. There are some cases of minor changes, or changes that affect features not present in a released version, where not having a dedicated changelog entry is ok.) There are two cases where forgetting to include something in the Changes has been problematic: - inline records were originally not part of the 4.03 Changelog (we caught the error as I gave a talk on new 4.03 features and Alain gently asked why I left inline records out of the discussion) - recently GPR ocaml#674 was forgotten from the Changelog If we make the Change mandatory in the CI, it is because without it there is no clear notification than it fails. It does not mean that having a Changelog entry is now mandatory, and it's just fine to keep deciding on a case-by-case basis not to have an entry. It's just that we would make that choice voluntarily, instead of out of distraction.
stedolan
pushed a commit
to stedolan/ocaml
that referenced
this pull request
Sep 21, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If
Sys.os_typeis set to an unknown value (e.g.xenin thecase of MirageOS), the Filename module currently raises an assertion
failure at initialisation time.
This patch alters Filename to default to Unix semantics if the
os_typeis unknown, therefore letting more progress be made.It is quite unlikely that
Filenamewill ever actually be usedin these embedded environments, but it is quite frequently linked
in due to being a stdlib dependency and being referencd from
something.
See also mirage/mirage#123