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

Misc. changes to import library generation tooling #2016

Merged
merged 5 commits into from Sep 13, 2022

Conversation

glandium
Copy link
Contributor

@glandium glandium commented Sep 9, 2022

This covers the first 3 points in #1964 (comment) and the first commit mentioned in the 4th point, that reduces differences from updating the msvc import libraries variants.

@kennykerr
Copy link
Collaborator

Handle both *_gnu and *_gnullvm import libraries from the same command

If you do this, you need to rerun cargo run -p tool_yml to rebuild the build scripts.

Copy link
Collaborator

@riverar riverar left a comment

Choose a reason for hiding this comment

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

Looks great, some minor changes requested.

crates/tools/gnu/src/main.rs Outdated Show resolved Hide resolved
crates/tools/msvc/src/main.rs Outdated Show resolved Hide resolved
@kennykerr
Copy link
Collaborator

What is the purpose of post processing the libs and is that really necessary?

@glandium
Copy link
Contributor Author

What is the purpose of post processing the libs

Assuming you use the same MSVC version, it makes the libs reproducible.

@kennykerr
Copy link
Collaborator

kennykerr commented Sep 12, 2022

Thanks, I would love reproducible builds but as the link Tim provided mentions, those aren't random timestamps but can and are often used for more than just uniquely stamping builds. I know the MSVC team are interested in this problem and will hopefully get this fixed before long. How about we just drop the post-processing until we've got more information? @oldnewthing

@glandium
Copy link
Contributor Author

What's random is what's in the actual SDK. What's in the import libs provided by windows-rs right now is an actual timestamp of when the tool_msvc tool is run. What's in the import libs generated by raw-dylibs by rust is an zeroed timestamp. Also, these "timestamps" appear nowhere in the binaries linked against those import libraries. I think you're overthinking this. The timestamps that matter more as per @oldnewthing's blog post are those on .dlls and .exes, not import libs.

@kennykerr
Copy link
Collaborator

What's random is what's in the actual SDK.

Well, that's not at all surprising. 🤣 I have no further objections. Thanks for your patience.

@riverar
Copy link
Collaborator

riverar commented Sep 12, 2022

We need to zoom out a bit and not assume these timestamps "random" and/or unnecessary. We don't have the data to support those assertions just yet. It's more likely these are internal implementation details (read: hacks) and are actually useful/used in some niche scenario.

I think this latest commit is a great best effort alignment of our generated libs to the official SDK libs, down to the -1!

@glandium Appreciate your patience and compromise.

@glandium
Copy link
Contributor Author

Can this be merged?

@kennykerr
Copy link
Collaborator

It looks like the gnullvm targets are still there. Should they be removed now?

@glandium
Copy link
Contributor Author

I'd rather get to that in a followup PR, especially since the modalities haven't been worked out yet.

@kennykerr kennykerr merged commit b5ee521 into microsoft:master Sep 13, 2022
@glandium
Copy link
Contributor Author

Thank you.

@glandium
Copy link
Contributor Author

I opened #2018 for discussion.

@kennykerr
Copy link
Collaborator

Great, thanks for the continued contributions!

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

Successfully merging this pull request may close these issues.

None yet

4 participants