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

Regression: cc_import not linked to target when added to deps of target #19056

Closed
chrisbrown2050 opened this issue Jul 25, 2023 · 16 comments
Closed
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules type: bug

Comments

@chrisbrown2050
Copy link

chrisbrown2050 commented Jul 25, 2023

Description of the bug:

Since bazelisk moved us to 6.3.0 yesterday, our libraries have not linked. This is a regression in 6.3.0 vs 6.2.0

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

http_archive('a')
cc_import(name='b', interface_library=b.lib, shared_library=b.dll)

cc_library(name='c_base', srcs=[x,y,z], deps=['@a//:b'])
cc_shared_library(name='c', deps=[":c_base"])

6.2.0: c_base links fine, linker params file includes b.lib
6.3.0: c_base not linked, linker params file is missing b.lib

Which operating system are you running Bazel on?

Windows

What is the output of bazel info release?

release 6.3.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

Between 6.2.0 and 6.3.0, will try to run bisect

Have you found anything relevant by searching the web?

No

Any other information, logs, or outputs that you want to share?

No response

@fmeum
Copy link
Collaborator

fmeum commented Jul 25, 2023

cc @oquenchil

@fmeum
Copy link
Collaborator

fmeum commented Jul 25, 2023

Your bisect is likely to land on 946777a, which combines multiple individual commits to master. Could you check whether the issue reproduces with last_green?

@sgowroji sgowroji added the team-Rules-CPP Issues for C++ rules label Jul 25, 2023
@chrisbrown2050
Copy link
Author

bazelisk --bisect does not work for me, is it supported by Windows bazelisk?

last_green gives me a different error:
Error in fail: Two shared libraries in dependencies link the same library statically. Both @//src/a:liba and @//src/b:libb link statically @//src:include_root

This diagnostic is correct, but unhelpful; src/include_root is a header-only library and thus has nothing to link.

@fmeum
Copy link
Collaborator

fmeum commented Jul 25, 2023

CC @meteorcloudy who implemented --bisect

@meteorcloudy
Copy link
Member

bazelisk --bisect does not work for me, is it supported by Windows bazelisk?

Yes, it should work for Windows. Can you provide the error messages you get while running bazelisk bisect?

@meteorcloudy meteorcloudy added P1 I'll work on this now. (Assignee required) and removed untriaged labels Jul 25, 2023
@keertk
Copy link
Member

keertk commented Jul 25, 2023

@bazel-io fork 6.3.1

@keertk
Copy link
Member

keertk commented Jul 25, 2023

@bazel-io fork 6.4.0

@oquenchil
Copy link
Contributor

I'm debugging this now. The code was never written to work like this because it discarded pre-compiled dynamic libraries (or at least that was the intention). So it was an accident that it worked for 6.2.

However, the bug report is valid because on Windows the transitive *.libs should be passed to the linker for DLL creation so this needs to be fixed. This doesn't apply on other platforms, for other platforms b shouldn't have to appear in the command line of c.

@oquenchil
Copy link
Contributor

Actually what I said in my previous comment is completely wrong. This is working at head by the way, I thought you had checked at last_green. This also affects Linux.

The issue here is that cc_import in 6.x doesn't advertise CcInfo when it should, i.e.: this line https://cs.opensource.google/bazel/bazel/+/master:src/main/starlark/builtins_bzl/common/cc/cc_import.bzl;drc=6ce7a5455640984f69c69d534db61d9c1873a9f0;l=212 is missing

@oquenchil
Copy link
Contributor

meteorcloudy pushed a commit that referenced this issue Jul 26, 2023
* Advertise CcInfo from cc_import

Fixes #19056

* Update BUILD.builtin_test
iancha1992 pushed a commit to iancha1992/bazel that referenced this issue Jul 26, 2023
* Advertise CcInfo from cc_import

Fixes bazelbuild#19056

* Update BUILD.builtin_test
iancha1992 added a commit that referenced this issue Jul 26, 2023
* Advertise CcInfo from cc_import

Fixes #19056

* Update BUILD.builtin_test

Co-authored-by: oquenchil <23365806+oquenchil@users.noreply.github.com>
@keertk
Copy link
Member

keertk commented Jul 26, 2023

@chrisbrown2050 Thank you for reporting this issue.
We're looking to improve our release process so that we can catch more issues early on in the cycle (before the final release) and would love your input. What can we do to make it easier for you to test release candidates in the future?

@meteorcloudy
Copy link
Member

@chrisbrown2050 Can you please confirm if this issue has been fixed in 6.3.1rc1?

@peakschris
Copy link

6.3.1rc1 does fix this issue - thank you!

As to how we could test release candidates, right now I don't track these. But now I know I can test them via bazelisk, it's pretty easy. How would I subscribe to notifications? Or can I set an environment var so my own machine updates to all the release candidates whilst all our production machines stay on official releases?

@meteorcloudy
Copy link
Member

meteorcloudy commented Aug 3, 2023

@peakschris Thanks for confirming! Yes, with bazelisk, you can point to the latest rc by setting USE_BAZEL_VERSION=last_rc, see https://github.com/bazelbuild/bazelisk#how-does-bazelisk-know-which-bazel-version-to-run

@keertk It's probably worth advertising this approach ;)

@meisterT
Copy link
Member

meisterT commented Sep 3, 2024

Can this be closed?

@peakschris
Copy link

From my POV, yes, I'm not seeing the issue anymore

@meisterT meisterT closed this as completed Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

No branches or pull requests

10 participants