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

Refactor building DLL example on Windows with windows_dll_library #7930

Closed

Conversation

meteorcloudy
Copy link
Member

@meteorcloudy meteorcloudy commented Apr 3, 2019

@excitoon Instead of #7629, what do you think of this one?
I would recommend to use cc_shared_library to build dll on Windows. One cc_shared_library can even depend on another.

@hlopko Do you have any plan to implement a cc_shared_library in https://github.com/bazelbuild/rules_cc?

@meteorcloudy meteorcloudy requested a review from hlopko April 3, 2019 14:26
@excitoon
Copy link
Contributor

excitoon commented Apr 4, 2019

@meteorcloudy This is a good solution, but I wanted a mechanism which would work with multiple extensions (.dll, .so, .dylib) and correspond to current target OS without 2 extra targets and in right way. Personally, I think this is kinda perfectionism, but this is how we do things.

So basically, as a part of plan I switch to one "wrong" (for 2 of 3 cases) extension, like we already have, but in parallel I would like to experiment with cc_common.link in order to write a rule which would link a binary with proper naming depending on target platform - #7538. Can you already tell if that is impossible that way? Or maybe can you provide any example to start hacking from?

BTW, we will keep waiting for C++ Sandwich and Transitive Linking anyway.

@meteorcloudy
Copy link
Member Author

I am not so familiar with the C++ Starlark API, so I'm not sure you can rename the output file name based on target platform. But if

artifact_name_pattern(
category_name = "dynamic_library",
prefix = "",
extension = ".dll",
),
is exposed in the API, we can definitely do that. @oquenchil can you confirm if it is available or not?

@meteorcloudy
Copy link
Member Author

meteorcloudy commented Apr 4, 2019

Oh, looks like it is!

@Param(
name = "artifact_name_patterns",
positional = false,
named = true,
defaultValue = "[]",
type = SkylarkList.class,
doc =
"A list of <a href=\"https://github.com/bazelbuild/bazel/blob/master/tools/cpp/"
+ "cc_toolchain_config_lib.bzl#L516\">artifact_name_patterns</a>."),

@hlopko
Copy link
Member

hlopko commented Apr 5, 2019

Re cc_shared_library, the tracking issue is #5200, and we plan to work on it in this quarter, definitely by Bazel 1.0.

Re naming, the api you found is what we use to replace CROSSTOOL, because that is where we specify extension names and prefixes (of output artifacts). With C++ sandwich API (#4570) which is being finished these days (and should be available in 0.26), you'll be able to create artifacts without specifying prefix and extension, and C++ toolchain will set those.

@meteorcloudy
Copy link
Member Author

@hlopko Do you think I can merge this change?

@hlopko
Copy link
Member

hlopko commented Apr 8, 2019

So why do you want to change this at all? cc_shared_library from examples is just an example, it's not supported, and we don't document or recommend people to use it anywhere. Up until this PR I completely forgot it existed :)

@meteorcloudy
Copy link
Member Author

Just to make the example more simple and clear.
People from EA actually asked about how to build DLL on Windows, I recommended this way since cc_shared_library is not yet available. Of course, I don't want to advertise it too much, but for people who want to use this way, this PR can help them having a better interface, which will make it easier to migrate to the proper cc_shared_library in Starlark in the future.

@meteorcloudy meteorcloudy changed the title Refactor building DLL example on Windows with cc_shared_library Refactor building DLL example on Windows with windows_dll_library Apr 8, 2019
@meteorcloudy
Copy link
Member Author

OK, renamed the macro to windows_dll_library to not cause any confusion with the future cc_shared_library

@meteorcloudy
Copy link
Member Author

@hlopko Does this PR look good now? :)

Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

Great! Thank you! :)

@bazel-io bazel-io closed this in c07188a Apr 15, 2019
buchgr pushed a commit to buchgr/bazel that referenced this pull request Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants