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

Workaround apparent bug in VS 2017.3 link #40

Merged
merged 1 commit into from Aug 30, 2017

Conversation

@dra27
Copy link
Collaborator

dra27 commented Aug 30, 2017

This is very weird - I can't see any in the VS 2017.3 release notes to suggest this change to the linker, but the error reported in #39 can be triggered by empty files, so I tried writing a single byte and the problem disappears!

I believe that the only reason that the implib is specified is so that flexlink can delete the .lib and .exp file cleanly, there are three other solutions:

  1. Sandbox the entire link command in a sub-directory of tmp and erase everything in it at the end (tedious amount of code...)
  2. Create the .exp file instead using temp_file and generate the .lib name from that (the linker doesn't seem to care about the .exp file existing)
  3. Create another file entirely and generate the .lib name from that

But I stopped at this solution, as it worked!

@alainfrisch
Copy link
Owner

alainfrisch commented Aug 30, 2017

Great, excellent. I'm always having cold sweats when I hear about flexdll being broken with new versions of VS...

Do you have an idea why link would try to open an existing file matching the implib target?

With the fix, have you been able to compile all of the OCaml distribution successfully with VS 2017.3?

@alainfrisch alainfrisch merged commit c1177e8 into alainfrisch:master Aug 30, 2017
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@dra27
Copy link
Collaborator Author

dra27 commented Aug 30, 2017

It's not just you - this one was more disturbing because it's an update, not a whole new release!

I have tested trunk msvc32 and msvc64, yes.

I can only imagine this has come from some kind of unintended side-effect - I rigged a test, for example, where I copied dllthreads.lib to the temporary file and built dllunix with no problem... the content didn't seem to matter, just that either the file was not present or it had at least 1 byte.

@bschommer
Copy link
Contributor

bschommer commented Aug 31, 2017

Maybe we should report this to the visual studio team. This seems really kind of strange.

@dra27
Copy link
Collaborator Author

dra27 commented Aug 31, 2017

Indeed - though we should reduce it to a minimal example first

@alainfrisch
Copy link
Owner

alainfrisch commented Aug 31, 2017

Would by any chance link /implib:foo.lib ... fail as soon as foo.lib is an existing empty file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.