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

yq: Fix path resolution for external targets #547

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phlax
Copy link

@phlax phlax commented Sep 24, 2023

Currently the yq target is unusable if called from an external build, this should resolve that


Type of change

  • Bug fix (change which fixes an issue)
  • New feature or functionality (change which adds functionality)
  • Style (white-space, formatting, etc...)
  • Refactor (a code change that neither fixes a bug or adds a new feature)
  • Performance (a code change that improves performance)
  • Documentation (updates to documentation or READMEs)
  • Chore (any other change that doesn't affect source or test files, such as configuration)

For changes visible to end-users

  • Breaking change (this change will force users to change their own code or config)
  • Relevant documentation has been updated
  • Suggested release notes are provided below:

Test plan

  • Covered by existing test cases
  • New test cases added
  • Manual testing; please provide instructions so we can reproduce:

@CLAassistant
Copy link

CLAassistant commented Sep 24, 2023

CLA assistant check
All committers have signed the CLA.

bin_dir = "/".join([ctx.bin_dir.path, ctx.label.package]) if ctx.label.package else ctx.bin_dir.path
bin_dir = ctx.bin_dir.path
if ctx.label.workspace_name:
bin_dir = "%s/external/%s" % (bin_dir, ctx.label.workspace_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't work with --nolegacy_external_runfiles.

@alexeagle
Copy link
Collaborator

It might be a good idea to file an issue for this first, in case others want to contribute a fix for it.

@phlax
Copy link
Author

phlax commented Sep 25, 2023

It might be a good idea to file an issue for this first, in case others want to contribute a fix for it.

i proposed a fix as this is blocking us from being able to do some external builds - so was hoping for a quick turnaround and needed something in the interim that works

#548

phlax added a commit to phlax/envoy that referenced this pull request Oct 24, 2023
upstream issue is bazel-contrib/bazel-lib#548
pr is: bazel-contrib/bazel-lib#547

Currently the websites and archive patch this indepenendently

This caused a breakage (in all 3) when aspect lib was updated

envoyproxy/envoy-website#368

This should prevent that until there is some upstream resolution

Signed-off-by: Ryan Northey <[email protected]>
phlax added a commit to envoyproxy/envoy that referenced this pull request Oct 24, 2023
bazel/patch: Patch aspect lib to resolve external build issue

upstream issue is bazel-contrib/bazel-lib#548
pr is: bazel-contrib/bazel-lib#547

Currently the websites and archive patch this indepenendently

This caused a breakage (in all 3) when aspect lib was updated

envoyproxy/envoy-website#368

This should prevent that until there is some upstream resolution

Signed-off-by: Ryan Northey <[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.

3 participants