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

Fix package-relative paths for external sources #128

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

bduffany
Copy link
Contributor

@bduffany bduffany commented Dec 28, 2022

Context: this unblocks migration of BuildBuddy to rules_js. We build our app externally like bazel build @com_github_buildbuddy_io_buildbuddy//enterprise/app:app_bundle when deploying via our internal repo, and this is broken because the current logic in rules_swc results in files being written to the wrong path.

Before, we'd get an error from esbuild like app.tsx: ERROR: Could not resolve "./root/root", and inspecting the sandbox dir after building with --sandbox_debug, the tree would look like this (filtering just to app.tsx and root.js here which are the main relevant files):

$(bazel-sandbox)/buildbuddy_internal/
    - bazel-out/k8-opt/bin/
        - external/com_github_buildbuddy_io_buildbuddy/
            - enterprise/app/
                - app.tsx  # esbuild entrypoint
                - external/com_github_buildbuddy_io_buildbuddy/enterprise/app/root/
                    - root.js # generated by SWC

The problem is that esbuild can't locate ./root/root.js because it's nested under ./external/com_github_buildbuddy_io_buildbuddy/enterprise/app/root/ relative to app.tsx, when it should just be under ./root/root.

This PR fixes that issue. After the fix, the directory structure looks like this:

$(bazel-sandbox)/buildbuddy_internal/
    - bazel-out/k8-opt/bin/
        - external/com_github_buildbuddy_io_buildbuddy/
            - enterprise/app/
                - app.tsx
                - root/
                    - root.js

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2022

CLA assistant check
All committers have signed the CLA.

@bduffany bduffany force-pushed the external branch 2 times, most recently from b55b2c8 to 6346fdf Compare December 28, 2022 19:44
@bduffany bduffany changed the title Fix paths for external sources Fix bin-relative paths for external sources Dec 28, 2022
@alexeagle
Copy link
Member

Thanks!
I'd really like to keep our helpers consolidated in https://docs.aspect.build/rules/aspect_bazel_lib/docs/paths especially since I think this bug is present in many rules. @kormide FWICT we don't have exactly this function there already? Should we add a new one?

Also note that next week we'll land #57 and will stop using short_path because we won't run swc under the nodejs binding anymore, and can follow "normal" bazel pathing idioms.

@alexeagle
Copy link
Member

Release v0.21.0 is published, I think that should solve this issue by removing the short_path logic altogether.

@bduffany
Copy link
Contributor Author

bduffany commented Jan 7, 2023

Release v0.21.0 is published, I think that should solve this issue by removing the short_path logic altogether.

Nice! I tested with v0.21.0, and it looks like we still need the fix for _relative_to_package, since without it, the package-relative relative source path for @com_github_buildbuddy_io_buildbuddy//enterprise/app/root:root.js is computed as ./external/com_github_buildbuddy_io/buildbuddy/enterprise/app/root/root.js when it should just be ./root.js relative to the package. But I reverted the other parts of the PR which are no longer necessary.

Side note, v0.21.0 is blazing fast 🔥 I can't even see the SWC compile actions in the progress output after a clean build.

@bduffany bduffany changed the title Fix bin-relative paths for external sources Fix package-relative paths for external sources Jan 7, 2023
@alexeagle alexeagle merged commit e2fb7e3 into aspect-build:main Jan 9, 2023
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.

None yet

3 participants