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

Collect implementation_deps in graph node aspect #14730

Closed

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Feb 4, 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 #14731

@fmeum fmeum marked this pull request as ready for review February 5, 2022 16:41
@fmeum fmeum requested a review from lberki as a code owner February 5, 2022 16:41
@fmeum fmeum force-pushed the cc-shared-library-implementation-deps branch from 1809779 to bd6b959 Compare February 9, 2022 08:27
@sgowroji sgowroji added the team-Rules-CPP Issues for C++ rules label Mar 24, 2022
@fmeum fmeum closed this Apr 3, 2022
@fmeum fmeum deleted the cc-shared-library-implementation-deps branch April 3, 2022 14:40
@fmeum fmeum restored the cc-shared-library-implementation-deps branch July 25, 2022 13:19
@fmeum fmeum reopened this Jul 25, 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.
@fmeum fmeum force-pushed the cc-shared-library-implementation-deps branch from ae000fc to 630a4a0 Compare September 10, 2022 12:57
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 10, 2022

@oquenchil I noticed that the starlark implementation has already been fixed. This PR still adds a test and a fix for the Java implementation. Is that useful?

Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

LGTM.

It definitely makes sense to merge the test because it's the only one and the rest as long as the code hasn't been deleted yet why not.

Thanks a lot.

@oquenchil oquenchil added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 16, 2022
@copybara-service copybara-service bot closed this in 21904a9 Oct 6, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 6, 2022
copybara-service bot pushed a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implementation_deps are not linked by cc_shared_library or cc_binary with dynamic_deps
3 participants