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

Remove printers_include with printers_lib #14442

Merged
merged 1 commit into from
Dec 17, 2020
Merged

Conversation

tkoeppe
Copy link
Contributor

@tkoeppe tkoeppe commented Dec 16, 2020

Commit Message: Remove the "printers_include" library in favour of "printers_lib".

Additional Description: The partial, header-only library :printers_include provided no service that wasn't already subsumed by :printers_lib.

Risk Level: low

Testing: in progress

Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

@tkoeppe tkoeppe force-pushed the header branch 2 times, most recently from 0dc7c8e to 66fa6ec Compare December 16, 2020 17:48
@tkoeppe
Copy link
Contributor Author

tkoeppe commented Dec 16, 2020

In this present draft, I'm removing :printers_include from the Starlark rule envoy_cc_test_library, for the following reason: the alternative is to change :printers_include to :printers_lib, as indeed many tests are using that dependency. However, a large number of tests are already directly depending on that rule, and simple duplication would be an error. The alternative would this be to remove the dependency from a large number of targets.

The present proposal is a bit simpler, since we just need to add the missing dependency in a handful of places.

Conceivably one could put more complicated deduplication logic into the Starlark rule, but that would seem to be unhealthy.

@alyssawilk, @qiwzhang: could you please advise on the preferred direction?

@alyssawilk
Copy link
Contributor

I don't have a strong opinion. @htuch and @lizan for build organization?

Alternately we could go for #2205 and try to remove this library entirely, but that's a larger change.

This change removes the defunct library printers_include and changes
printers_lib to export the printers.h header. The Starlark rule
envoy_cc_test_library no longer adds the printer dependency, and
instead the now-missing dependencies on the new printers_lib target
are added to a small number of targets that need them.

Signed-off-by: Thomas Köppe <[email protected]>
@tkoeppe tkoeppe changed the title WIP remove printers_include Remove printers_include with printers_lib Dec 16, 2020
@htuch
Copy link
Member

htuch commented Dec 17, 2020

I think the only reason to have separation would have been some cyclic dependency, so this makes sense.

@alyssawilk alyssawilk merged commit 5e6c4b9 into envoyproxy:master Dec 17, 2020
@tkoeppe
Copy link
Contributor Author

tkoeppe commented Dec 17, 2020

@htuch: Perhaps, though in my experience you can never really work around circular dependencies like that -- try bazel build --dynamic_mode=off to make sure. Typically you'd just have to put all the TUs that use the circular dependency into a single cc_library rule.

@tkoeppe tkoeppe deleted the header branch December 17, 2020 22:15
mpuncel added a commit to mpuncel/envoy that referenced this pull request Dec 22, 2020
* master: (30 commits)
  Deflaked: Guarddog_impl_test (envoyproxy#14475)
  [fuzz] add fuzz tests for hpack encoding and decoding (envoyproxy#13315)
  [filters] Prevent a filter from sending local reply and continue (envoyproxy#14416)
  oauth2: improving coverage (envoyproxy#14479)
  owners: Change dio email address (envoyproxy#14498)
  macos build: Fix ninja install (envoyproxy#14495)
  http: use OptRef helper to reduce some boilerplate (envoyproxy#14361)
  doc: update test/integration/README.md (envoyproxy#14485)
  server: wait workers to start before draining parent. (envoyproxy#14319)
  api: relax inline_string length limitation in DataSource (envoyproxy#14461)
  oauth: properly stop filter chain when a response was sent (envoyproxy#14476)
  listener: deprecate use_proxy_proto (envoyproxy#14406)
  deps: update cel and remove a patch (envoyproxy#14473)
  preconnect: rename: (envoyproxy#14474)
  coverage: ratcheting limits (envoyproxy#14472)
  grpc mux: fix sending node again after stream is reset (envoyproxy#14080)
  [test] Replace printers_include with printers_lib. (envoyproxy#14442)
  tcp: nodelay in the new pool (envoyproxy#14453)
  test: replace mock_methodn macros with mock_method (envoyproxy#14450)
  tcp: extending tcp integration test (envoyproxy#14451)
  ...

Signed-off-by: Michael Puncel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants