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

implementation_deps are not linked by cc_shared_library or cc_binary with dynamic_deps #14731

Closed
fmeum opened this issue Feb 5, 2022 · 2 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: bug

Comments

@fmeum
Copy link
Collaborator

fmeum commented Feb 5, 2022

Description of the problem:

Both cc_shared_library and cc_binary with dynamic_deps do not link in transitive dependencies that they are only connected to through implementation_deps.

The root cause is that both the Starlark and the Java version of the graph aspect only consider attributes named deps.

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

See the integration test in #14730.

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

development version

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

From 5c4422e via bazel build //src:bazel-dev.

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

https://github.com/bazelbuild/bazel
5c4422e
5c4422e

Have you found anything relevant by searching the web?

No

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

I have prepared a fix at #14730.

@oquenchil oquenchil added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Feb 14, 2022
@fmeum fmeum closed this as not planned Won't fix, can't repro, duplicate, stale Jul 24, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Jul 24, 2022

Closing as implementation_deps are no more. I will check whether interface_deps suffer from the same problem.

@oquenchil
Copy link
Contributor

Hold on Fabian. I got feedback a few months ago that interface_deps is an incompatible change across repos which is uncalled for given the little benefit it provides (it was about consistency with the exports attribute in other rules). I thought I could solve this via WORKSPACE magic and I tried last week but I can't. Taking that into account I will probably rollback the change that converted implementation_deps to interface_deps.

Also I suspect that the same problem applies to interface_deps since it's probably related to the aspect not being propagated to those attributes. The fix shouldn't be too complicated but I will do it once I have reverted the previous change.

@fmeum fmeum reopened this Jul 25, 2022
copybara-service bot pushed a commit that referenced this issue Oct 11, 2022
*** Reason for rollback ***

Due to #16296 (comment)

*** Original change description ***

Collect implementation_deps in graph node aspect

This is needed for implementation_deps of cc_library targets to be linked into cc_binary targets with dynamic_deps and cc_shared_library targets.

Fixes #14731

Closes #14730.

PiperOrigin-RevId: 480360695
Change-Id: Ic1b9c17ce3af730fa0131f5d87e5ca99ae2740c5
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This is needed for implementation_deps of cc_library targets to be linked into cc_binary targets with dynamic_deps and cc_shared_library targets.

Fixes bazelbuild#14731

Closes bazelbuild#14730.

PiperOrigin-RevId: 479253298
Change-Id: I933f2e9fc3171378cc95a50c59a426489b8724c3
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
*** Reason for rollback ***

Due to bazelbuild#16296 (comment)

*** Original change description ***

Collect implementation_deps in graph node aspect

This is needed for implementation_deps of cc_library targets to be linked into cc_binary targets with dynamic_deps and cc_shared_library targets.

Fixes bazelbuild#14731

Closes bazelbuild#14730.

PiperOrigin-RevId: 480360695
Change-Id: Ic1b9c17ce3af730fa0131f5d87e5ca99ae2740c5
fweikert pushed a commit that referenced this issue Oct 20, 2022
*** Reason for rollback ***

Due to #16296 (comment)

*** Original change description ***

Collect implementation_deps in graph node aspect

This is needed for implementation_deps of cc_library targets to be linked into cc_binary targets with dynamic_deps and cc_shared_library targets.

Fixes #14731

Closes #14730.

PiperOrigin-RevId: 480360695
Change-Id: Ic1b9c17ce3af730fa0131f5d87e5ca99ae2740c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants