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

Use the object crate to generate .def files #402

Closed
wants to merge 2 commits into from

Conversation

folkertdev
Copy link
Contributor

These files are relatively simple, and using a rust library rather than an external shell command is convenient for testing and cross-compilation. I also suspect that for integrating into cargo/rustc, calling an external binary won't be accepted.

Is this logic exercised by CI? If not I could add an example .dll and test that it generates the expected .def file.

@lu-zero
Copy link
Owner

lu-zero commented Sep 10, 2024

rustc already calls parts of the platform toolchain so it wouldn't be that problematic, the part of the CI that build the examples should fail if the .def is wrong.

Using object makes the code simpler so it is good, I wonder if object itself couldn't directly get the feature of producing the .def files...

@lu-zero
Copy link
Owner

lu-zero commented Sep 10, 2024

If that test weren't omitted on windows. I guess now it is a good time to cover that.

@lu-zero
Copy link
Owner

lu-zero commented Sep 10, 2024

@amyspark do you have time to look into enabling building the example on the window targets?

@amyspark
Copy link
Contributor

No problem, I'll be happy to look into that.

@amyspark
Copy link
Contributor

@lu-zero I ended up finding a bug in pkgconf: pkgconf/pkgconf#364.

Apart from that, @folkertdev you need to modify https://github.com/lu-zero/cargo-c/blob/master/src/build.rs#L1155-L1163. Currently, the output of Rust/MSVC is used verbatim, the line

note: native-static-libs: kernel32.lib advapi32.lib ntdll.lib userenv.lib ws2_32.lib /defaultlib:msvcrt

must be converted to

-lkernel32 -ladvapi32 -lntdll -luserenv -lws2_32 /defaultlib:msvcrt

for pkg-config to output good libraries.

Once that's fixed, adding the commit from folkertdev/cargo-c@use-object-for-exports...amyspark:cargo-c:use-object-for-exports should suffice for it to work.

@lu-zero
Copy link
Owner

lu-zero commented Sep 11, 2024

That is something that should be reported and fixed upstream I'm afraid. What could use that link line verbatim?

@folkertdev
Copy link
Contributor Author

I'm a bit confused what needs to happen here now. #404 was merged, which includes the commits from this PR.

The pkgconf problem was reported, so do you want to hold off on further changes until that is resolved?

Apart from that, @folkertdev you need to modify https://github.com/lu-zero/cargo-c/blob/master/src/build.rs#L1155-L1163. Currently, the output of Rust/MSVC is used verbatim, the line

note: native-static-libs: kernel32.lib advapi32.lib ntdll.lib userenv.lib ws2_32.lib /defaultlib:msvcrt

must be converted to

-lkernel32 -ladvapi32 -lntdll -luserenv -lws2_32 /defaultlib:msvcrt

for pkg-config to output good libraries.

the location you link to does not mention that line at all? So what is the problem there exactly, what must be changed?

@lu-zero
Copy link
Owner

lu-zero commented Sep 11, 2024

It can be closed, the patch was folded in the other set.

@sdroege
Copy link
Sponsor Contributor

sdroege commented Sep 12, 2024

Not sure it makes sense to replace 70 lines of code with a crate that is 500 times that big :)

@folkertdev
Copy link
Contributor Author

interesting. To me not having to call an external executable is worth a lot.

  • spawning a process is slow
  • parsing stdout/stderr is error-prone
  • setting up the arguments is messy
  • running commands is impossible to cross-compile

If raw size is a concern, there are a bunch of feature flags that can be turned off for the object crate, significantly reducing its size.

@sdroege
Copy link
Sponsor Contributor

sdroege commented Sep 12, 2024

I'm mostly concerned about pulling in quite a bit of code that is maintained elsewhere (how well? I don't know) and re-implements functionality of the official toolchain (is it bug-to-bug compatible etc?).

@sdroege
Copy link
Sponsor Contributor

sdroege commented Sep 12, 2024

running commands is impossible to cross-compile

Also that's something that meson has solved quite nicely with the exe wrapper (so you can go via qemu or wine or whatever is necessary for executing things). cargo doesn't have any such features unfortunately and is generally a lot more messy with regards to cross-compilation.

@folkertdev
Copy link
Contributor Author

In this case, reading the exports of a PE/COFF file is a fairly standard operation. The object crate is used in rustc itself and many other projects besides, so as a dependency I'd say it seems fairly safe.

I've reduced the feature flags here #410

Also that's something that meson has solved quite nicely

well if it doesn't work with cargo it kinda doesn't work...

Having just one build tool is incredibly valuable, and while it's imperfect in many ways, if compatibility with that approach is as easy as using a fairly standard crate to parse a binary format from the previous century, that seems like a win to me.

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.

4 participants