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

Unnecessary recommendation for "linkstatic=1" #310

Closed
fweikert opened this issue Jul 16, 2015 · 6 comments
Closed

Unnecessary recommendation for "linkstatic=1" #310

fweikert opened this issue Jul 16, 2015 · 6 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug
Milestone

Comments

@fweikert
Copy link
Member

Bazel sometimes suggests to use linkstatic=1, even though it is not necessary.

Original error report: https://groups.google.com/forum/#!topic/bazel-discuss/XcmwEnJSDNU

@gregestren gregestren added the P3 We're not considering working on this, but happy to review a PR. (No assignee) label Jul 16, 2015
@gregestren
Copy link
Contributor

To be clear, this occurs because of: https://github.com/google/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java#L382-385

Conceptually, we can't remove all ambiguity for configurable attributes because in general one path may include object files while another doesn't (and we don't know when the warning is triggered which path will be chosen). But we can more finely tune it for cases like described in the original path, where all paths have object files (or all paths don't).

The only complication is the link above gets a https://github.com/google/bazel/blob/master/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java to examine attribute values. It'll need an https://github.com/google/bazel/blob/master/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java instead to have the ability to iterate over all paths.

@bsilver8192
Copy link
Contributor

Actually, after a bit of investigation I concluded that the issue is something different. When rule.isConfigurable("srcs", Type.LABEL_LIST) is true, the problem doesn't occur because the caller of appearsToHaveNoObjectFiles (https://github.com/google/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java#L330) doesn't trigger the warning.

The problem is when there is something in srcs like a filegroup which gives different files depending on the configuration, which doesn't result in rule.isConfigurable("srcs", Type.LABEL_LIST) being true. I'm not sure if there is some simple way to detect that case and add that to the if on line 330, but I'm pretty sure that would solve my issue.

@gregestren
Copy link
Contributor

Brian,

Can you show a repro case? (e.g. a specific bazel invocation combined with the example you outlined in the referenced groups link above)? It's not clear to me now this has anything to do with select. e.g. I think we can get the same behavior with something straightforward like filegroup(name = 'my_filegroup', srcs = [''empty.h])

I think the more immediate distinction between having a filegroup in the sources vs. a direct source file, e.g. empty.h, is that the latter won't trigger on this check:

https://github.com/google/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java#L401

@gregestren
Copy link
Contributor

@bsilver8192
Copy link
Contributor

I can't reproduce this any more. I don't remember exactly which version I was using, but I am only seeing the warning in configurations which don't have any .cc files in srcs with a recent one. Before, I (at least though I) was seeing the warning in all configurations, even ones in which there was a .cc file in srcs. I think this bug should be closed now. Thanks for looking into it.

Some extra details:
A simple case like you mentioned (filegroup(name = 'my_filegroup', srcs = [''empty.h])) does trigger the warning in the current version and probably would in the original one too, but it's correct.

That comment at https://github.com/google/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java#L366 disagrees with what I have observed about the actual behavior; I haven't seen any way in which the name of a filegroup affects whether the warning appears or not.

The actual rules where I was hitting the warning seem to have been some combination of this issue and an intermediate cc_library I missed before which actually had only headers in one configuration.

@kayasoze
Copy link

kayasoze commented Aug 6, 2015

There is another case in which this warning is spurious: a cc_library() which contains a .a or .so (or both) in srcs[]. This configuration is useful when wrapping an autoconf or cmake generated library to pick up include paths.

@ulfjack ulfjack added this to the 1.0 milestone Jun 15, 2016
bazel-io pushed a commit that referenced this issue Aug 12, 2021
  - 3c7ec07fe0418446ffdb1a04c671a3810d74ae30 [cas] Add cas package (#300) by nodirg <[email protected]>
  - 28fa42989a6c2d05cddb6b42494f34ef742c3de9 [cas] Implement file reading (#302) by nodirg <[email protected]>
  - 69c6642c3a006636256c1d2e591899f07d4c74bf Simplify caching packages (#303) by nodirg <[email protected]>
  - f7087af662fe5481f2e75abd652af46d9a376534 [cas] Implement presence check (#304) by nodirg <[email protected]>
  - 847bca232f884b1e7ee059c64317a5140f11477e Simplify caching packages further (#306) by nodirg <[email protected]>
  - 989513ef4a567a812f21b27f2a5d83f1a2145600 Rename singleflightcache.Cache (#310) by nodirg <[email protected]>
  - 7eceb37537dec7c50e52d4f1af1a5d671c267144 [cas] Fix the build (#308) by nodirg <[email protected]>
  - fd877b05ba2ed611f6d34e711da05917b729eff9 [cas] Add retries (#311) by nodirg <[email protected]>
  - 5bc303584ef03ded33f03fa2976015e38da9c050 [cas] Add support for Symlinks. (#309) by nodirg <[email protected]>
  - 4cfed65947cba54af1b2259d9e3facf7dd3007b9 [cas] Implement batch upload (#307) by nodirg <[email protected]>
  - 144126c43e73aeeabdbeb8bc4ef0290fa7a91ba7 [cas] Move file IO semaphore to Client (#313) by nodirg <[email protected]>
  - e1da041171a715b7bab3178acfe1cd774b9f5019 [cas] Reuse file read buffers (#317) by nodirg <[email protected]>
  - e9184e44947661852a8887f3183e00c346208aaa [cas] Add UploadOptions (#314) by nodirg <[email protected]>
  - e2bd6c8e2d6bc183ff059c688a604ba1ac18b840 [cas] Implement ServerCapabilities check (#315) by nodirg <[email protected]>
  - b00d91e726265f23ad0a88730545831b8d5519ef [cas] Add RPCConfig (#318) by nodirg <[email protected]>
  - 1c678dec65b62e49840419ab777c7b6ce65cfd76 [cas] Improve error messages (#321) by nodirg <[email protected]>
  - ad8d2cfffe1f3728469a8dd5a7531b9157280ecd [cas] Limit FindMissingBlobs concurrency (#319) by nodirg <[email protected]>
  - b1b54ee4d55b5d5bd71ca2df4353058515581697 [cas] Increase FindMissingBlobs concurrency to 256 (#323) by nodirg <[email protected]>
  - b4a0e12d87c946ced360d723d743be1f57a47995 [cas] Move file IO buffering deeper (#322) by nodirg <[email protected]>
  - abb14633e09633368f06d8e4fd28bd1553509061 [cas] Add filtering/callback (#316) by nodirg <[email protected]>
  - 7447b28dd69e22848ee850936a1cdd28e2d0e20b Add Mtime to the file metadata cache. (#326) by ramymedhat <[email protected]>
  - 7182b476eb6260fae9ba2bd8995b97a7096e340c [cas] Implement streaming (#320) by nodirg <[email protected]>
  - b0605647bbe2ff7d046a286c3023e7714376fb83 include file path in upload error (#327) by Takuto Ikuta <[email protected]>
  - 3b602dd48f7f63a76cb1087a10355b3c668d583f [cas] Implement per-request timeouts in a stream (#325) by nodirg <[email protected]>
  - 395c674af7a9cd696dfd1f2b4a950f6899ccb3a0 remove unused variables (#332) by Takuto Ikuta <[email protected]>
  - 45f49a9529f755fb586fe2f8bf3e78b1eae39e81 [cas] Unembed cas.Client.ClientConfig. (#333) by nodirg <[email protected]>
  - 2a9b29928abe867026e37833fa480ed16238df7a [cas] Rename UploadOptions.Callback to Prelude (#334) by nodirg <[email protected]>
  - 80ea864b211ee3d87e14f4be0ca8d17e5917062c [cas] Read files once (#335) by nodirg <[email protected]>
  - 0e577525a2dce2d0e7bddd80927c8901d28bf0fb [cas] PathExclude: use forward-slash-separated paths (#341) by nodirg <[email protected]>
  - e155d015bcc4c9eb9978572422e3404f26220700 Add useful error message for uploading files. (#339) by bansalvinayak <[email protected]>
  - 3dfb518d390280a2ffef5a85ab1852a2a526a983 [cas] Rename UploadInput to PathSpec (#342) by nodirg <[email protected]>
  - 752e4efb2631b45f5c27120f83d517cc2b2846d2 google/uuid -> pborman/uuid (#344) by Rubens Farias <[email protected]>
  - d94f8a8ba888d384686a7cd74d8a9d0795ba4b6d Small tweaks to appease internal import checks. (#346) by Rubens Farias <[email protected]>
  - f9d52cdef1c3aa8612d9c3ed7a0b65f96e55d870 Catch another pool check (#347) by Rubens Farias <[email protected]>
  - ead1458eda2b7c756138429121a3e22bd5c9aa5a Preserving symlink or not can be configured from Command.... by Yoshisato Yanagisawa <[email protected]>
  - f831c118b9c9e1dd3e857fbe43da7c992c91b20f make (*Chunker).Reset returns error (#348) by Takuto Ikuta <[email protected]>
  - 1a7d2a4198fa0eb8515593b5259f495cdacf75ab [cas] Require PathSpec.Path to be absolute (#345) by nodirg <[email protected]>
  - dd2d3976ed7c6482f361de7d24e24d5a8683c56c Upgrade zstdpool-syncpool for DecoderWrapper.Close bugfix... by Mostyn Bramley-Moore <[email protected]>
  - 8544bdc0f3112900675a72decdb7a978cece0be7 [cas] Clean PathSpec.Path (#352) by nodirg <[email protected]>
  - 882e3342509eb038dc04bf44581e5085c0320d16 [cas] Add UploadResult.Digest() (#340) by nodirg <[email protected]>
  - 5d4d813411299a285f113cb541f2c5750746cdfa chunker: remove unused field from Chunker (#355) by Takuto Ikuta <[email protected]>
  - a5af2d4316599a3fea87a459290e5f88a022a43c Add return value names in singleflightcache (#356) by nodirg <[email protected]>
  - e7ea26b93b496d4d30e98390682faf6f69f84cb7 [cas] Fix joinFilePathsFast (#361) by nodirg <[email protected]>
  - 5a8daf747858747b2bf6bdd59e7b07dedc17a244 [cas] Refactor code (#359) by nodirg <[email protected]>
  - dd6c290b2ce791f3a3e56583a9632f7b538e8a05 [cas] Simplify UploadResult.Digest() signature (#362) by nodirg <[email protected]>
  - 05222e7e8939959878a5798e51cf52e911feafaf Fix lint (#357) by nodirg <[email protected]>
  - b2689fabc306d2cd20356f30b7f99eec445d9212 [cas] Refactor Digest() (#363) by nodirg <[email protected]>
  - c672e5baca9280d181c7e2e87b3d9afd01650893 [cas] Add PathSpec.Allowlist (#360) by nodirg <[email protected]>
  - f9e6595d5634ac4d4221ba6dca9f5c2e64114b3f add size check (#365) by Takuto Ikuta <[email protected]>
  - 3d0cf1be08dd52d77204ad1be7e70fdd1e48c206 Revert "Add useful error message for uploading files. (#3... by Rubens Farias <[email protected]>
  - 3ddc89f3e2b39308101060b3eeaa10a919cae13e "Wrap" gRPC error codes. (#367) by Rubens Farias <[email protected]>
  - d965bf95d0af9d88d90e9723aee6b332dc3ef93e cas: fix deadlock (#368) by Takuto Ikuta <[email protected]>
  - 3c4ce9170b6c5a5d64bcc4feecc5bcdf5dd1f101 allow to use streamBufSize larger than 32KiB (#369) by Takuto Ikuta <[email protected]>
  - 21d6adc44e550f7aa5c4e9b3fedab838920a9632 use semaphore for large file upload (#370) by Takuto Ikuta <[email protected]>
  - 1cec173a5bf76c02f435050d6fc9a02e1ccea637 update remote-apis (#371) by Takuto Ikuta <[email protected]>
  - e96eb06339fb616167ee02535ab54d9c3a382232 add more log around upload (#372) by Takuto Ikuta <[email protected]>
  - 3f34e744d83161ddcb602121b45b0b33185a36c5 Make glog import consistent. (#373) by Rubens Farias <[email protected]>
  - 3db822c86088434a4d2d53ec2d3b889b9f8cf331 Remove typos (#374) by Rubens Farias <[email protected]>

PiperOrigin-RevId: 390319167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) type: bug
Projects
None yet
Development

No branches or pull requests

6 participants