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.

Sign up for GitHub

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 executable compilation with select #2506

Merged
merged 1 commit into from
Aug 5, 2019
Merged

Fix executable compilation with select #2506

merged 1 commit into from
Aug 5, 2019

Conversation

bobot
Copy link
Collaborator

@bobot bobot commented Aug 4, 2019 •
edited
Loading

Since #2268 the compilation of an executable with a select is broken because the not selected .ml are still compiled.

@bobot bobot requested a review from rgrinberg August 4, 2019 16:57
@bobot
Copy link
Collaborator Author

bobot commented Aug 4, 2019

Not fixed yet. I'm not able to find the best way to fix it.

@rgrinberg
Copy link
Member

Did you forget to commit the entire test? I don't see a dune file.

@bobot
Copy link
Collaborator Author

bobot commented Aug 4, 2019

It is an old tests that was just in 1.0 that I bump in 2.0 to see the problem.

@rgrinberg
Copy link
Member

Ah I see. I suggest that we test both cases however.

As for a fix, a real simple approach would be to just make this behavior controllable via a flag in link_exe (rather than just based on the dune version). If there are selects for this executable, then we can simply choose the old behavior.

@bobot
Copy link
Collaborator Author

bobot commented Aug 4, 2019

We know the selected select case statically, since we know in a super_context which libraries are available. So could we remove the module of the other case from the list of modules? I was looking for something special for the select case, but perhaps we currently don't compile them just because they are not used by other modules.

@rgrinberg
Copy link
Member

Yeah that should work too. I'm still wondering how those extra sources are being compiled, the conditionally selected sources aren't actual modules.

@bobot bobot force-pushed the select_fix_compilation branch from 948d64a to cd2c445 Compare August 4, 2019 18:49
@bobot bobot changed the title [WIP] fix executable compilation with select Fix executable compilation with select Aug 4, 2019
@bobot
Copy link
Collaborator Author

bobot commented Aug 4, 2019

It works. I think it is safe to do the modification for any dune version.

@bobot
Fix (libraries (select ... with 2.0
e8db51d 
   The source files used in select are removed from the list of files in the
   directory.

Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot bobot force-pushed the select_fix_compilation branch from cd2c445 to e8db51d Compare August 4, 2019 20:18
rgrinberg
rgrinberg approved these changes Aug 5, 2019
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks Francois.

@bobot bobot merged commit fa5ed53 into master Aug 5, 2019
@bobot bobot mentioned this pull request Nov 6, 2019
Closed
@Alizter Alizter deleted the select_fix_compilation branch November 12, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg 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.
2 participants
@bobot @rgrinberg