fix: always include files from the same workspace as the build target in copy_to_directory()
#360
Conversation
dgp1130
force-pushed
the
external-copy-directory
branch
from
70b3d6b
to
58700cc
Compare
dgp1130
force-pushed
the
external-copy-directory
branch
from
58700cc
to
54bf1a2
Compare
|
Thanks for the PR @dgp1130 . I can help with the sequencing after I find a bit of time to review this code. We've been really busy with Workflows sales in the last month so haven't had as much time as I wanted for OSS work. |
|
No worries @gregmagolan. Would it be helpful for me to split this into two PRs? One for the Go binary and one with the Starlark changes? That way you could merge them independently. |
|
Hey @dgp1130 sorry this got parked like this. Yeah if you're willing to make the Golang change in a PR, I'll review/merge/release with that, then the starlark will be able to rely on it being there. Note to @gregmagolan - I have the same problem in container-structure-test now too. I know we've brainstormed some ways to make the golang library "live from HEAD" but it's probably not time to design that yet. |
dgp1130
force-pushed
the
external-copy-directory
branch
3 times, most recently
from
0588a71
to
6fdac2c
Compare
dgp1130
force-pushed
the
external-copy-directory
branch
from
6fdac2c
to
1fb14b9
Compare
|
No problem @alexeagle, I'm sure you've had more important things to worry about than this. I broke up the Go changes into a separate PR and made the new value optional to avoid breaking anything. Once that gets merged and released, we should be able to rebase this and merge it with the changes to the It's not very important, but IMHO the new workspace name option in the other PR should be required. Unfortunately doing that would break the |
dgp1130
force-pushed
the
external-copy-directory
branch
from
1fb14b9
to
0d93810
Compare
… in `copy_to_directory` Fixes aspect-build#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
force-pushed
the
external-copy-directory
branch
3 times, most recently
from
f84c925
to
2d313ee
Compare
…workspace Refs aspect-build#359. This should catch regressions where no files are copied when built within an external workspace and not using `include_external_repositories`.
|
Since #488 got merged, I rebased this PR so it should be mostly good to go. Unfortunately it seems like the CI setup has changed and it isn't running the new workspace properly. Not sure how this is supposed to work or why the directories seem to be missing, but hopefully someone can help point me in the right direction here? |
|
Somehow you had an extra /workspace segment on your new e2e folder and an extra e2e/workspace entry as well. Added a commit to fix that |
|
Thanks! Glad we're able to land this fix! |
… 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 <alex@aspect.dev>
… 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 <alex@aspect.dev>
Fixes #359.
This updates the
copy_to_directorytool to accept the workspace name of the target it is executing under and automatically include all source files under that workspace (after apply other filters) regardless ofinclude_external_repositories.I believe all the existing tests should pass, as they do when I manually update
copy_to_directory()to use the version of the tool built from HEAD. However they don't do that by default, and adding this workspace name option is a breaking change to the existing contract of thecopy_to_directorytool. It needs to be released at the same time as this fix, unfortunately I'm not sure how to do that or if I even can. @gregmagolan, any suggestions on how to move forward here?