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

Jump to bottom

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

Merged
merged 3 commits into from Jul 23, 2019
Merged

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

merged 3 commits into from Jul 23, 2019

Conversation

lpw25
Copy link
Contributor

@lpw25 lpw25 commented Jul 19, 2019

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

gasche
gasche reviewed Jul 19, 2019
View reviewed changes
otherlibs/dynlink/byte/dynlink.ml
List.filter
(fun id ->
not (Ident.is_predef id)
&& not (String.contains (Ident.name id) '.'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gasche gasche Jul 19, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
View reviewed changes
Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

dra27 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
Copy link
Member

gasche commented Jul 20, 2019 •
edited

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
Copy link
Member

gasche commented Jul 20, 2019

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

@dra27
Copy link
Member

dra27 commented Jul 20, 2019 •
edited

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
Copy link
Member

gasche 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
Copy link
Member

dra27 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
Copy link
Member

gasche 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
Copy link
Contributor

XVilka 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
gasche added a commit to gasche/ocaml that referenced this pull request Jul 23, 2019
@gasche
Merge pull request ocaml#8818 from lpw25/fix-dynlink-packs
1c606bf 
Fix ocaml#8816 (Dynlink/packing issue in bytecode with 4.08)

(cherry picked from commit e14a611)
@gasche gasche mentioned this pull request Jul 23, 2019
Merged
gasche added a commit that referenced this pull request Jul 23, 2019
@gasche
Merge pull request #8822 from gasche/backport-bytecode-dynlink-fix-in…
101383a 
…-4.08

Backport #8818 (Dynlink/packing issue in bytecode) for 4.08+rc2
dra27 pushed a commit that referenced this pull request Jul 30, 2019
@gasche @dra27
Merge pull request #8822 from gasche/backport-bytecode-dynlink-fix-in…
0ef82b2 
…-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
Reviewers

@gasche gasche gasche approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dynlink/packing issue in bytecode with 4.08
4 participants
@lpw25 @dra27 @gasche @XVilka