Skip to content
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

Assume that module names that are not in `Env.t` are persistent #2235

Merged
merged 7 commits into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@diml
Copy link
Member

commented Feb 4, 2019

When loading a cmi file, we use a made up environment to interpret the global identifiers in this file. Additionally, since #2041 identifiers must be explicitly marked as persistent via Env.add_persistent_structure in order to be considered as persistent modules. This means that the made up environment must have the various global identifiers referenced in the cmi file marked as persistent.

In #2041, we made the assumption that this list of global identifiers corresponded to the list of imports written in the cmi file (Cmi_format.cmi_crcs). However, this is not true in the presence of module aliases. For instance:

$ echo 'module X = Y' > alias.ml
$ echo -e 'open Alias\nmodule Z = X' > foo.ml
$ ocamlc -no-alias-deps -w -49 -c alias.ml
$ ocamlc -no-alias-deps -w -49 -c foo.ml
$ ocamlobjinfo foo.cmi|grep Y
<no match>

This is an issue as it breaks the compilation of anything using module aliases to namespace libraries.

This PR proposes to fix this issue by considering that any module that is not found at all in the environment is in fact a persistent module. This way, all global identifiers in cmi files will correctly be treated as persistent ones. It is no longer necessary to build the made up environment in acknowledge_pers_struct so I just used the empty environment instead.

This patch needs to go in 4.08.

Show resolved Hide resolved typing/env.ml Outdated
@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Also: can you add your test(s) to the testsuite?

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2019

Happy to write a test, however I'm not too sure how to proceed.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Happy to write a test, however I'm not too sure how to proceed.

Can't you produce a simplified version of what's happening on ocaml/dune#1792 ?

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

it seems easy to write as a shell cram test. How can I write such a test with the current test infrastructure?

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

You can launch a shell script from an ocamltest script:

foo.ml:

(* TEST
  script = "${test_source_directory}/foo.sh"
  * setup-ocamlc.byte-build-env
  ** script
 *)
Printf.printf "hello\n"

foo.sh:

#!/bin/sh
ocamlc foo.ml && ./a.out
@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

@damiendoligez thanks. However, the ocamlc I have access to inside the script is not the right one. It is the one accessible from the PATH of my shell rather than the newly built one.

For instance, if I do ocamlc -version inside the script, I get 4.07.1.

diml added some commits Feb 5, 2019

Add test for #2235
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Fix bug introduced by #2041
The environment used to lookup global identifiers coming from loaded
cmi files was incomplete, leading to identifiers that could not be
resolved.

This patch fixes the issue by assuming that module names that are not
found in the environment are always external.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

@diml diml force-pushed the diml:fix-bug branch from e17044b to 800f2a5 Feb 5, 2019

@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

You don't really need to run a script though.
You could just have all your .ml files in your test directory, and call the compiler via ocamltest as is done for various other tests.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

How do I do that?

FTR, this is my test:

#!/bin/sh

set -e

cat > a.ml <<EOF
let x = 42
EOF

cat > lib__.ml <<EOF
module A = Lib__A
EOF

cat > lib.ml <<EOF
module A = A
EOF

cat > user_of_lib.ml <<EOF
open Lib
let x = A.x
EOF

ocamlc -no-alias-deps -w -49 -c lib__.ml
ocamlc -no-alias-deps -w -49 -open Lib__ -o lib__A.cmo -c a.ml
ocamlc -no-alias-deps -w -49 -open Lib__ -o lib.cmo -c lib.ml
ocamlc -no-alias-deps -w -49 -c user_of_lib.ml
@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

You do some variation of this.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

Sorry, I have very superficial knowledge of ocamltest and I don't really understand what test_locations.ml is doing or how to encode my test in ocamltest comments.

I did the following manual testing: I tried to build dune with both trunk and this PR. It fails with trunk but passes with this PR. Do you mind if I don't add a test in the testsuite at this point? I'm happy to add a test in the future once I understand ocamltest better.

code review
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Show resolved Hide resolved typing/env.ml Outdated
@damiendoligez

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

I'd be happy to get this PR merged without a test for the moment.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

Do you mind if I don't add a test in the testsuite at this point?

I do actually.
I agree it would be nice if ocamltest was documented, but given that there are already multiple examples in the test suite, and that I pointed you to one, I have no doubt that can hack something that works :)

Btw, your test is nice for the issue you're intending to fix, but I think you should also add the test of MPR#6886, since it looks like the initial version of this GPR was reintroducing that bug.

_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

The result would look less readable that the script anyway, so I'd rather make the script version work.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

I’ll try to see if the test can be done more neatly, but I agree with @trefis that their presence is important (the tests I added to #2041 were painful to write too...).

Ultra-minor nit, but this GPR should be added to 2041 in Changes, rather than suppressing the CI test.

_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

Thanks @dra27! I updated the changelog. I definitely agree that such tests are important, though given that this is time sensitive, we might have to merge the fix without a test initially to not delay 4.08.

trefis added some commits Feb 6, 2019

Revert "Add test for #2235"
This reverts commit 80a8882.
@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

I’ll try to see if the test can be done more neatly

Thanks @dra27, but it's OK, I added the test.
I also removed the non-working test which called out to sh for no reason.

I'll add a test for MPR#6886 in a separate PR.

@trefis

trefis approved these changes Feb 6, 2019

@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Given the state of the history, I suggest that whoever merges this (when the CIs come back green) does a squash merge.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

Thanks @trefis!

@trefis trefis merged commit 5c828b5 into ocaml:trunk Feb 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

I merged onto trunk. I'll let someone else cherry-pick on 4.08.

@diml diml deleted the diml:fix-bug branch Feb 7, 2019

diml added a commit that referenced this pull request Feb 7, 2019

Assume that module names that are not in `Env.t` are persistent (#2235)
Fix bug introduced by #2041

The environment used to lookup global identifiers coming from loaded
cmi files was incomplete, leading to identifiers that could not be
resolved.

This patch fixes the issue by assuming that module names that are not
found in the environment are always external.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

I cherry-picked the commit to 4.08

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Thanks guys, now I can do the beta!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.