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

Fix #8816 (Dynlink/packing issue in bytecode with 4.08) #8818

Merged
merged 3 commits into from Jul 23, 2019

Conversation

@lpw25
Copy link
Contributor

commented Jul 19, 2019

This fixes #8816 by only including toplevel modules in the list of implementation imports of .cmo files.

List.filter
(fun id ->
not (Ident.is_predef id)
&& not (String.contains (Ident.name id) '.'))

This comment has been minimized.

Copy link
@gasche

gasche Jul 19, 2019

Member

Without knowing the context, this looks a bit strange. Where is the code that creates predefined identifiers with . in their name?

This comment has been minimized.

Copy link
@gasche

gasche Jul 19, 2019

Member

(Well I guess those are the Ident.create_persistent (packagename ^ "." ^ name) calls in bytecomp/bytepackager.ml.)

This comment has been minimized.

Copy link
@lpw25

lpw25 Jul 19, 2019

Author Contributor

Yeah, and you'll also notice a use of String.contains ... '.' in that file as well. It's not pleasant, but there are a couple of PRs coming down the pipeline that will tidy up this stuff so I'm not in a hurry to fix it right now.

@gasche

gasche approved these changes Jul 19, 2019

Copy link
Member

left a comment

The fix is not beautiful but it is simple and well-tested -- and it works. I agree with @lpw25's approach of getting the simplest fix out first, and then making things more robust in trunk.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

I was going to push a Changes entry, but it's actually non-obvious where it goes!

@Octachron - given the report against 4.08.1+rc1, do you want this on 4.08?

@gasche - there's no section in trunk Changes for 4.08.1 - or should the numbers just be added to the original Dynlink soundness entry in 4.08.0, and put in different sections on the 4.08 and 4.09 branches??

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Yes, we want this in 4.08.1. We have a "4.08 maintenance branch" section in the 4.08 branch, we need to import it in the other banches. Let me take care of that right now.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

(Wouldn't it be nice if we had tooling support to more easily work with Changes entries? Just kidding.)

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

You kid - I did a few months ago start to resurrect the tool I wrote in 2017, but it needs more than an hour or so's work to resurrect, so it sits on the spike for now until I have something really important I should be doing and need distracting from 😉

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

I put the Changes entries in order, all post-4.08 branches should now have a "4.08 maintenance" section.

@dra27 dra27 force-pushed the lpw25:fix-dynlink-packs branch from f4ebe86 to 713f1e8 Jul 20, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2019

Thanks @gasche - I've rebased and pushed a Changes entry, so I think is ready squash and pick once CI reconfirms.

@gasche gasche changed the title Fix #8816 Fix #8816 (Dynlink/packing issue in bytecode with 4.08) Jul 23, 2019

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

If you allow a very minor nitpick, "Fix #N" is not a very good name for a PR. When I have to find them again in the PR list, such titles make me unhappy. I edited this one and the other one (I just repeated/summarized the name of the bug they are fixing); let's avoid them in the future.

@gasche gasche closed this Jul 23, 2019

@gasche gasche reopened this Jul 23, 2019

@XVilka

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

By the way, it makes sense to suggest GitHub autocomplete the PR title with the issue title itself, once you type 'Fix #xxxx' in the PR title. Would solve exactly this problem.

@gasche gasche merged commit e14a611 into ocaml:trunk Jul 23, 2019

2 checks passed

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

gasche added a commit to gasche/ocaml that referenced this pull request Jul 23, 2019

Merge pull request ocaml#8818 from lpw25/fix-dynlink-packs
Fix ocaml#8816 (Dynlink/packing issue in bytecode with 4.08)

(cherry picked from commit e14a611)

gasche added a commit that referenced this pull request Jul 23, 2019

Merge pull request #8822 from gasche/backport-bytecode-dynlink-fix-in…
…-4.08

Backport #8818 (Dynlink/packing issue in bytecode) for 4.08+rc2

dra27 added a commit that referenced this pull request Jul 30, 2019

Merge pull request #8822 from gasche/backport-bytecode-dynlink-fix-in…
…-4.08

Backport #8818 (Dynlink/packing issue in bytecode) for 4.08+rc2

(cherry picked from commit 101383a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.