create_library_to_link: add mangle_name and unique_name arguments by aherrmann · Pull Request #8888 · bazelbuild/bazel

aherrmann · 2019-07-15T11:14:54Z

Adds mangle_name and unique_name arguments to create_library_to_link
- mangle_name allows users replicate the behavior of cc_library in their custom rules. The link placed under the solib directory will be mangled and no intermediate directory will be generated.
- unique_name allows users to specify that the given library name is unique and no intermediate directory is required.
Adds the prefixLibraryPath argument to the internal CcCommon API to facilitate the above changes to create_library_to_link.
Adds shell tests for the mangle_name and unique_name arguments to create_library_to_link.

Motivation

Before this PR the new Starlark Cc API's create_library_to_link would always preserve the given library name and link it into the solib directory underneath an intermediate directory based on the library label and consumer label. These intermediate directories can grow very long, in particular if external workspaces are involved. In rules_haskell this causes issues on Windows and MacOS. On Windows the linker cannot look for libraries underneath search paths which exceed a certain path length. On MacOS the header size of a MACH-O binary/library is limited and the required runpaths/load commands would quickly exceed this limit due to the long paths generated by Bazel. In rules_haskell in particular these intermediate directories are unnecessary, as rules_haskell already mangles the library names itself to ensure their uniqueness. Furthermore, the Haskell compiler GHC expects library names of a particular shape, such that Bazel's builtin mangling cannot be used. This motivates the combination mangle_name = False, unique_name = True enabled by this PR.

cc @oquenchil @mboes @cocreature

aherrmann · 2019-07-15T17:27:54Z

CI has passed and this is now ready for review.

The tests test the symlinks generated in the solib directory. Since these are not generated on Windows, I have disabled the tests on Windows.

mboes · 2019-08-10T18:50:52Z

This PR just exposes existing method parameters that are there in the Java API to Starlark code. It's currently a blocker for a few issues in rules_haskell.

oquenchil · 2019-08-13T08:01:03Z

I will take a look at this on Monday next week. Even though this is in Java, it doesn't mean we want to expose it to Starlark. We are carefully reviewing every parameter we add to the API because unlike in Java fixing a mistake there is not as simple as an internal refactoring of our code.

oquenchil · 2019-08-19T14:35:47Z

So if I understand correctly, you want more flexibility in the name used for the symlink created. I think better than the two booleans on the API, the change should add a String parameter which internally gets converted to a PathFragment. Then internally override the method getDynamicLibrarySymlink with a variation that doesn't take preserveName and prefixConsumer, but takes that PathFragment instead and where getMangledNamed() is not called.

That should give enough flexibility to do what you want. But first I have to ask, why was it causing issues to have the default preserveName=true and prefixConsumer=true. For a given instance, what was the name of the symlink produced and what was the name that you wanted and why did you want that different name?

Regarding the test, I would prefer if you remove the shell test altogether and add a new test here:
https://github.com/bazelbuild/bazel/blob/master/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java

Thanks for this PR!

aherrmann · 2019-08-21T11:22:57Z

So if I understand correctly, you want more flexibility in the name used for the symlink created.

Yes, that is correct.

I think better than the two booleans on the API, the change should add a String parameter which internally gets converted to a PathFragment. Then internally override the method getDynamicLibrarySymlink with a variation that doesn't take preserveName and prefixConsumer, but takes that PathFragment instead and where getMangledNamed() is not called.

Interesting, that sounds feasible, I'll look into it.

But first I have to ask, why was it causing issues to have the default preserveName=true and prefixConsumer=true. For a given instance, what was the name of the symlink produced and what was the name that you wanted and why did you want that different name?

Sure, take tweag/rules_haskell@21c277b with Bazel 0.27.0 on Linux with the default GHC bindist. Then run bazel build //tests/binary-with-lib-dynamic.
This will produce

$ ls bazel-bin/_solib_k8/*
bazel-bin/_solib_k8/_U@rules_Uhaskell_S_Shaskell_Ctoolchain-libraries___Uexternal_Srules_Uhaskell_Ughc_Ulinux_Uamd64_Slib_Sbase-4.12.0.0:
libHSbase-4.12.0.0-ghc8.6.5.so

bazel-bin/_solib_k8/_U@rules_Uhaskell_S_Shaskell_Ctoolchain-libraries___Uexternal_Srules_Uhaskell_Ughc_Ulinux_Uamd64_Slib_Sghc-prim-0.5.3:
libHSghc-prim-0.5.3-ghc8.6.5.so

bazel-bin/_solib_k8/_U@rules_Uhaskell_S_Shaskell_Ctoolchain-libraries___Uexternal_Srules_Uhaskell_Ughc_Ulinux_Uamd64_Slib_Sinteger-gmp-1.0.2.0:
libHSinteger-gmp-1.0.2.0-ghc8.6.5.so

bazel-bin/_solib_k8/_U@rules_Uhaskell_S_Shaskell_Ctoolchain-libraries___Uexternal_Srules_Uhaskell_Ughc_Ulinux_Uamd64_Slib_Srts:
libffi.so  libHSrts-ghc8.6.5.so

bazel-bin/_solib_k8/_U_S_Stests_Sbinary-with-lib-dynamic_Clib___Utests_Sbinary-with-lib-dynamic:
libHStestsZSbinary-with-lib-dynamicZSlib-ghc8.6.5.so

By default Bazel produces very long directory names, and separate directories for most libraries. On macOS, the standard GHC bindist requires dynamic linking as part of compilation, each of these long paths will have to appear in the MACH-O header as a runpath or load command. This can very quickly add up to exceed the MACH-O header size limit. The header may not exceed 32k. With paths like the above we have frequently had issues with builds failing due to MACH-O header size at around 200 library dependencies. Currrently, rules_haskell employs a number of workarounds to avoid the issue. Shortening the symlink paths as with this PR would make our lives as rules_haskell maintainers easier, and also raise the upper bound for the number of possible library dependencies.

For core Haskell libraries as well as haskell_library targets, the directory prefix is unnecessary. The first few libraries are core libraries shipped with the compiler toolchain. Their paths look like this:

_U@rules_Uhaskell_S_Shaskell_Ctoolchain-libraries___Uexternal_Srules_Uhaskell_Ughc_Ulinux_Uamd64_Slib_Sbase-4.12.0.0/libHSbase-4.12.0.0-ghc8.6.5.so

and should instead look like this

libHSbase-4.12.0.0-ghc8.6.5.so

The library name contains the name and version of the corresponding core package and the compiler version. This makes accidental name collisions very, very unlikely.

The last library comes from a local haskell_library target. Its path looks like this:

_U_S_Stests_Sbinary-with-lib-dynamic_Clib___Utests_Sbinary-with-lib-dynamic/libHStestsZSbinary-with-lib-dynamicZSlib-ghc8.6.5.so

and should instead look like this

libHStestsZSbinary-with-lib-dynamicZSlib-ghc8.6.5.so

The library name already contains the full package path and is thereby unique. The libHS prefix is required by the Haskell compiler, same as the compiler version suffix.

Regarding the test, I would prefer if you remove the shell test altogether and add a new test here:
https://github.com/bazelbuild/bazel/blob/master/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java

Right, I'm not too familiar with the internal Java API. If I understand correctly, I'd generate the necessary files similar to how it's done in setUpCcLinkingContextTest and then test the symlinks similar to how it's done here. Correct?

Thanks for the review @oquenchil

Adds an overload of `getDynamicLibrarySymlink` that takes the relative path of the symlink to be created underneath the solib directory. Addresses review comment bazelbuild#8888 (comment)

By default the Starlark function `create_library_to_link` would always preserve the library name and create a subdirectory underneath `solibDir` to place the library symlink. This commit allows to override this behavior by specifying a path fragment where to place the libray symlink underneath the solib dir. Addresses review comment bazelbuild#8888 (comment)

Add tests for custom solib link paths in create_library_to_link. Addresses review comment bazelbuild#8888 (comment)

aherrmann · 2019-08-22T13:35:40Z

@oquenchil I've addressed your comments. create_library_to_link now takes optional arguments dynamic_library_path and interface_library_path. If set, these override the path of the symbolic link underneath the solib directory. I've also removed the shell integration tests and instead added Java tests in SkylarkCcCommonTest.java as requested. PTAL.

oquenchil · 2019-08-22T14:00:55Z

Yes, that looks perfect. One little nit though. The dynamic_library_path and interface_library_path are changing the paths only for the symlinks correct? Ideally, these two names would make that clear, so something like dynamic_library_symlink_path and interface_library_symlink_path.

Otherwise, I think it might cause confusion with people thinking that the path of the actual dynamic_library and interface_library gets replaced.

Also initially I wanted to complain about symlinks being an internal optimization detail which is platform dependent and shouldn't be leaked. But this is already leaking through the resolved_symlink_dynamic_library and resolved_symlink_dynamic_library getters, so no issue here.

The PR looks perfect, the test is exactly how it should be and I appreciate you took good care of limiting the filetypes for these two parameters. Just renaming the two input parameters to make clear that it's for the symlink and we can merge this.

Addresses review comment bazelbuild#8888 (comment)

aherrmann · 2019-08-22T17:20:25Z

@oquenchil Thank you, that's great to hear!

The dynamic_library_path and interface_library_path are changing the paths only for the symlinks correct? Ideally, these two names would make that clear, so something like dynamic_library_symlink_path and interface_library_symlink_path.

Yes, that is correct. Agreed, makes sense to be more explicit in those names. I've changed the names as suggested. PTAL

oquenchil

LGTM

c-parsons · 2019-08-27T15:30:51Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java

+        @Param(
+            name = "dynamic_library_symlink_path",
+            doc =
+                "Override the default path of the dynamic library link in the solib directory.",


Path is an actual Starlark type -- lets make this clear this should be a string instead of a path type? maybe "path string" ?
(Same below)

I've amended the docstring following #8888 (comment). That should clarify this a bit more. Also, the html documentation will display the type String above that description, similar to here.

c-parsons · 2019-08-27T15:31:06Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java

+            named = true,
+            noneable = true,
+            type = String.class,
+            defaultValue = "None"),


Is there a possibility empty-string is a meaningful path here? Otherwise, consider using empty string as the default value, and making the parameter type an actual String instead of an Object.

You're right that an empty string is not meaningful here. I was trying to follow the convention of None meaning "use default behavior". But, it looks like for strings "" is indeed used for that purpose, e.g. in repository_ctx.execute(..., working_directory = ""), so this change seems alright. The advantage is that it makes it easier to forward a default value from a rule string attribute, like here.

Addressing review comment bazelbuild#8888 (comment)

aherrmann

@c-parsons I've addressed your comments. PTAL

aherrmann · 2019-08-28T09:27:09Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java

+        @Param(
+            name = "dynamic_library_symlink_path",
+            doc =
+                "Override the default path of the dynamic library link in the solib directory.",


I've amended the docstring following #8888 (comment). That should clarify this a bit more. Also, the html documentation will display the type String above that description, similar to here.

aherrmann · 2019-08-28T09:30:26Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java

+            named = true,
+            noneable = true,
+            type = String.class,
+            defaultValue = "None"),


You're right that an empty string is not meaningful here. I was trying to follow the convention of None meaning "use default behavior". But, it looks like for strings "" is indeed used for that purpose, e.g. in repository_ctx.execute(..., working_directory = ""), so this change seems alright. The advantage is that it makes it easier to forward a default value from a rule string attribute, like here.

aherrmann · 2019-09-27T09:27:44Z

@c-parsons @oquenchil Can this be merged?

c-parsons

Sorry this review fell off my radar In general, this looks good to me. But I'd like to get approval from oquenchil@ before proceeding, as he has more knowledge of this piece of the API.

c-parsons · 2019-09-27T18:19:54Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java

+    PathFragment dynamicLibraryPathFragment = null;
+    if (dynamicLibraryPath != null && !dynamicLibraryPath.isEmpty()) {
+      dynamicLibraryPathFragment = PathFragment.create(dynamicLibraryPath);
+      if (dynamicLibraryPathFragment.isEmpty() || dynamicLibraryPathFragment.isAbsolute() || dynamicLibraryPathFragment.containsUplevelReferences()) {


Do we need to check dynamicLibraryPathFragment.isEmpty(), given that we already checked dynamicLibraryPath.isEmpty above?

Good question. PathFragment.create performs normalization if necessary. The docs don't seem to guarantee that normalization never returns an empty PathFragment given a non-empty String. E.g. ./ or foo/.. could potentially be normalized away. For reference, the docs say

* <p>Strings are normalized with '.' and '..' removed and resolved (if possible), any multiple * slashes ('/') removed, and any trailing slash also removed. Windows drive letters are uppercased. * The current implementation does not touch the incoming path string unless the string actually * needs to be normalized.

c-parsons · 2019-09-27T18:25:10Z

src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java

+    assertThat(
+            librariesToLink.toCollection(LibraryToLink.class).stream()
+                .filter(x -> x.getDynamicLibrary() != null)
+                .map(x -> x.getDynamicLibrary().getRootRelativePath().relativeTo(toolchain.getSolibDirectory()).toString())


nit: can you refactor x.getDynamicLibrary().getRootRelativePath().relativeTo(toolchain.getSolibDirectory()).toString() to a helper private static method, to make this slightly more readable? (the combination of chained calls and use of stream() makes this difficult to read)

I've factored it out as getSolibRelativePath.

bazelbuild#8888 (comment)

oquenchil · 2019-10-07T12:12:21Z

Sorry this review fell off my radar In general, this looks good to me. But I'd like to get approval from oquenchil@ before proceeding, as he has more knowledge of this piece of the API.

Yes, still looks good to me.

mjrussell · 2019-10-30T07:02:54Z

Im running into MACH-O limits in rules_haskell and would greatly appreciate this getting merged. Is there anything holding this up from merging?

aherrmann · 2019-10-30T09:16:56Z

@c-parsons I think this is ready for merge. Let me know if there's something else you'd like to see changed in this PR.

@mjrussell Thanks for pinging. Regarding MACH-O limits, which version of rules_haskell are you on? Version 0.11 contains some workarounds that should avoid most MACH-O header size issues. If you're still hitting these issues on 0.11, could you open an issue at https://github.com/tweag/rules_haskell with some more details?

src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java

src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java

Adds an overload of `getDynamicLibrarySymlink` that takes the relative path of the symlink to be created underneath the solib directory. Addresses review comment bazelbuild#8888 (comment)

By default the Starlark function `create_library_to_link` would always preserve the library name and create a subdirectory underneath `solibDir` to place the library symlink. This commit allows to override this behavior by specifying a path fragment where to place the libray symlink underneath the solib dir. Addresses review comment bazelbuild#8888 (comment)

Add tests for custom solib link paths in create_library_to_link. Addresses review comment bazelbuild#8888 (comment)

Addresses review comment bazelbuild#8888 (comment)

Addressing review comment bazelbuild#8888 (comment)

bazelbuild#8888 (comment)

Addresses bazelbuild#8888 (comment)

aherrmann · 2019-11-21T14:36:34Z

@c-parsons I've addressed your comments. PTAL.

c-parsons · 2019-11-26T22:00:22Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java


-    Artifact notNullArtifactForIdentifier = null;
    StringBuilder extensionErrorsBuilder = new StringBuilder();
    String extensionErrorMessage = "does not have any of the allowed extensions";


It's still used in the validation logic for the static and dynamic library attributes. E.g. here. Sadly, it doesn't match so well with the symlink validation and refactoring that code too much seems out of scope for this PR.

c-parsons · 2019-11-26T22:00:46Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java

  }

+  private static void validateSymlinkPath(Location location, String attrName, PathFragment symlinkPath, FileTypeSet filetypes, StringBuilder errorsBuilder) throws EvalException {
+    String extensionErrorMessage = "does not have any of the allowed extensions";


please just inline this string

bazelbuild#8888 (comment)

c-parsons

Will merge shortly

aherrmann · 2019-12-17T10:08:02Z

Will merge shortly

Awesome, thank you!

aherrmann requested a review from hlopko as a code owner July 15, 2019 11:14

googlebot added the cla: yes label Jul 15, 2019

aherrmann mentioned this pull request Jul 15, 2019

Allow to override preserveName and prefixConsumer in create_library_to_link #8180

Closed

aherrmann force-pushed the create_library_to_link branch from 51cdeec to 6e3e4e2 Compare July 15, 2019 12:53

aiuto added team-Rules-CPP Issues for C++ rules untriaged labels Jul 15, 2019

aherrmann force-pushed the create_library_to_link branch 2 times, most recently from 2fface6 to f85baf2 Compare July 15, 2019 16:36

hlopko requested review from oquenchil and removed request for hlopko July 16, 2019 12:24

iirina removed the untriaged label Jul 17, 2019

jin assigned lberki Jul 22, 2019

lberki assigned oquenchil Jul 22, 2019

mboes mentioned this pull request Aug 10, 2019

libffi not found when a cc_binary depends on a haskell_library and using bindists tweag/rules_haskell#1032

Closed

oquenchil unassigned lberki Aug 13, 2019

aherrmann-da pushed a commit to digital-asset-archive/bazel that referenced this pull request Aug 22, 2019

SkylarkCcCommonTest: Custom solib link path

4de84d0

Add tests for custom solib link paths in create_library_to_link. Addresses review comment bazelbuild#8888 (comment)

aherrmann force-pushed the create_library_to_link branch from f85baf2 to 4de84d0 Compare August 22, 2019 12:45

aherrmann requested a review from scentini as a code owner August 22, 2019 12:45

aherrmann-da pushed a commit to digital-asset-archive/bazel that referenced this pull request Aug 22, 2019

Rename to dynamic_library_symlink_path

7e2fe28

Addresses review comment bazelbuild#8888 (comment)

oquenchil approved these changes Aug 23, 2019

View reviewed changes

c-parsons reviewed Aug 27, 2019

View reviewed changes

aherrmann-da pushed a commit to digital-asset-archive/bazel that referenced this pull request Aug 28, 2019

*_library_symlink_path: Default to ""

e89bddc

Addressing review comment bazelbuild#8888 (comment)

aherrmann commented Aug 28, 2019

View reviewed changes

c-parsons reviewed Sep 27, 2019

View reviewed changes

aherrmann added a commit to aherrmann/bazel that referenced this pull request Sep 30, 2019

Factor out getSolibRelativePath

85c0a8e

bazelbuild#8888 (comment)

guibou mentioned this pull request Nov 6, 2019

Remove system specific information from stable-status.txt #10177

Closed

c-parsons requested changes Nov 8, 2019

View reviewed changes

aherrmann added 10 commits November 20, 2019 17:49

Allow user-defined dynamic library symlink

797ddb6

Adds an overload of `getDynamicLibrarySymlink` that takes the relative path of the symlink to be created underneath the solib directory. Addresses review comment bazelbuild#8888 (comment)

SkylarkCcCommonTest: Custom solib link path

d09e7cf

Add tests for custom solib link paths in create_library_to_link. Addresses review comment bazelbuild#8888 (comment)

Rename to dynamic_library_symlink_path

6ec1e9d

Addresses review comment bazelbuild#8888 (comment)

*_library_symlink_path: Default to ""

be23afc

Addressing review comment bazelbuild#8888 (comment)

Factor out getSolibRelativePath

6c97d9b

bazelbuild#8888 (comment)

Avoid .toString in String.format

db67648

Addresses bazelbuild#8888 (comment)

Use Strings.isNullOrEmpty

67e0fc2

Addresses bazelbuild#8888 (comment)

Check for _solib_ inputs

83fb5f0

Addresses bazelbuild#8888 (comment)

Factor out symlinkPath validation

43dc9a7

Addresses bazelbuild#8888 (comment)

aherrmann force-pushed the create_library_to_link branch from 85c0a8e to 43dc9a7 Compare November 21, 2019 14:07

c-parsons reviewed Nov 26, 2019

View reviewed changes

Inline error string

6132110

bazelbuild#8888 (comment)

c-parsons approved these changes Dec 16, 2019

View reviewed changes

bazel-io closed this in 5824d24 Dec 17, 2019

aherrmann mentioned this pull request Apr 22, 2020

Shorter dynamic library symlinks underneath _solib_<cpu> tweag/rules_haskell#1311

Merged

Conversation

aherrmann commented Jul 15, 2019

Motivation

Uh oh!

aherrmann commented Jul 15, 2019

Uh oh!

mboes commented Aug 10, 2019

Uh oh!

oquenchil commented Aug 13, 2019

Uh oh!

oquenchil commented Aug 19, 2019

Uh oh!

aherrmann commented Aug 21, 2019

Uh oh!

aherrmann commented Aug 22, 2019

Uh oh!

oquenchil commented Aug 22, 2019

Uh oh!

aherrmann commented Aug 22, 2019

Uh oh!

oquenchil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aherrmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aherrmann commented Sep 27, 2019

Uh oh!

c-parsons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oquenchil commented Oct 7, 2019

Uh oh!

mjrussell commented Oct 30, 2019

Uh oh!

aherrmann commented Oct 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aherrmann commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c-parsons left a comment

Choose a reason for hiding this comment

Uh oh!

aherrmann commented Dec 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects