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

[Bug]: copy_to_directory_bin_action() creates an empty directory when run in an external workspace. #359

Closed
1 task
dgp1130 opened this issue Feb 10, 2023 · 0 comments · Fixed by #360
Closed
1 task
Labels
bug Something isn't working

Comments

@dgp1130
Copy link
Contributor

dgp1130 commented Feb 10, 2023

What happened?

More context: https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1676010753876439

npm_package() from @aspect_rules_js doesn't include sources from its own workspace when built in an external workspace. Instead, you get an empty directory (more context). It seems that the include_external_repositories filter of copy_to_directory is a little overzealous and automatically includes sources from the __main__ workspace, not the workspace containing the target. I think the correct behavior here is to automatically include any files in the same workspace as the npm_package() target, and I think that also applies to the underlying copy_to_directory_bin_action().

I think the right fix is to just make copy_to_directory_bin_action() add a --workspace_name flag containing the name of the workspace for the current target and pass that into the copy_to_directory tool. Then the tool should always include files within that workspace regardless of include_external_repositories (after applying other filters ofc).

I have a failing test case in dgp1130@33e3ed9, but haven't started on a proper fix yet.

Version

Reproducible in latest version of @aspect_bazel_lib (1.24.2).

How to reproduce

# my_wksp/BUILD.bazel

npm_package(name = "pkg", srcs = ["file.txt"])

Then link that from another workspace:

# my_other_wksp/WORKSPACE.bazel

local_repository(
    name = "my_wksp",
    path = "../my_wksp",
)

Then build:

(cd my_other_wksp/ && bazel build @my_wksp//:pkg)

It will output an empty directory.

Any other information?

My current workaround is to add include_external_repositories = ["my_wksp"] to npm_package(), but it's not ideal and requires that any downstream workspaces refer to this package's workspace as @my_wksp when they should be free to choose any name.

Fund our work

  • Sponsor our open source work by donating a bug bounty
@dgp1130 dgp1130 added the bug Something isn't working label Feb 10, 2023
@github-actions github-actions bot added the untriaged Requires traige label Feb 10, 2023
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Feb 11, 2023
… in `copy_to_directory`

Fixes bazel-contrib#359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Feb 11, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Feb 11, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Feb 11, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/rules_prerender that referenced this issue Feb 12, 2023
Refs #48.

This actually worked a lot easier than I expected. `link_prerender_component()` worked just fine and the only real hiccup was adding `include_external_repositories = ["rules_prerender"]` to work around bazel-contrib/bazel-lib#359.
@gregmagolan gregmagolan removed the untriaged Requires traige label Feb 13, 2023
@gregmagolan gregmagolan moved this to 🔖 Ready in Open Source Feb 13, 2023
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
… in `copy_to_directory`

Fixes bazel-contrib#359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
… in `copy_to_directory`

Fixes bazel-contrib#359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
…pace and automatically include files from it

Refs bazel-contrib#359.

This allows `copy_to_directory` to be run in an external workspace and include files from that workspace automatically once the `copy_to_directory` rule is updated. The rule update needs to be separated from the tool update because the tool is pre-built and vendored rather than built at HEAD.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
… in `copy_to_directory`

Fixes bazel-contrib#359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
… in `copy_to_directory`

Fixes bazel-contrib#359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Jul 30, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
alexeagle pushed a commit that referenced this issue Sep 21, 2023
…pace and automatically include files from it (#488)

Refs #359.

This allows `copy_to_directory` to be run in an external workspace and include files from that workspace automatically once the `copy_to_directory` rule is updated. The rule update needs to be separated from the tool update because the tool is pre-built and vendored rather than built at HEAD.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Oct 10, 2023
… in `copy_to_directory`

Fixes bazel-contrib#359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Oct 10, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Oct 10, 2023
… in `copy_to_directory`

Fixes bazel-contrib#359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Oct 10, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Oct 10, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Oct 10, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
dgp1130 added a commit to dgp1130/aspect_bazel_lib that referenced this issue Oct 10, 2023
…workspace

Refs bazel-contrib#359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
alexeagle added a commit that referenced this issue Oct 10, 2023
… in `copy_to_directory()` (#360)

* fix: always include files from the same workspace as the build target in `copy_to_directory`

Fixes #359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).

* test: add e2e test which uses `copy_to_directory` within an external workspace

Refs #359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.

* ci: fix stray workspace refs

---------

Co-authored-by: Alex Eagle <[email protected]>
@github-project-automation github-project-automation bot moved this from On Deck to ✅ Done in Open Source Oct 10, 2023
alexeagle added a commit that referenced this issue Oct 10, 2023
… in `copy_to_directory()` (#360)

* fix: always include files from the same workspace as the build target in `copy_to_directory`

Fixes #359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).

* test: add e2e test which uses `copy_to_directory` within an external workspace

Refs #359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.

* ci: fix stray workspace refs

---------

Co-authored-by: Alex Eagle <[email protected]>
alexeagle added a commit that referenced this issue Dec 23, 2023
… in `copy_to_directory()` (#360)

* fix: always include files from the same workspace as the build target in `copy_to_directory`

Fixes #359.

This updates the `copy_to_directory` tool to accept a workspace name representing the workspace of the target it is executing under. Any files in this workspace are automatically included, regardless of the `include_external_repositories` option. This makes it support usage within an external target (such as `@wksp//:dir`).

* test: add e2e test which uses `copy_to_directory` within an external workspace

Refs #359.

This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.

* ci: fix stray workspace refs

---------

Co-authored-by: Alex Eagle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants