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

npm install: get rid of platform dependent cp command #1797

Closed
wants to merge 2 commits into from

Conversation

deining
Copy link
Collaborator

@deining deining commented Jan 21, 2024

This PR follows up on the discussion we have had in #1793.
It alters the mount points for npm boostrap dependency. After this change, the platform dependent cp command which does not run on linux, can be discarded, thus solving #1793.

@deining deining force-pushed the npm-no-cp branch 7 times, most recently from 92727bb to 4e8ad9f Compare January 21, 2024 15:43
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I'll take a look as soon as I can when back at the office the week of the 29th. In the meantime, could you ensure that all of the GH actions pass?

@deining deining force-pushed the npm-no-cp branch 4 times, most recently from 7ff10ab to 0fb00a7 Compare January 21, 2024 22:22
@deining
Copy link
Collaborator Author

deining commented Jan 21, 2024

Thanks for this PR! I'll take a look as soon as I can when back at the office the week of the 29th. In the meantime, could you ensure that all of the GH actions pass?

All GH actions are passing now 😄. Looking forward to your comments!

@deining deining requested a review from chalin January 21, 2024 22:28
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Oh, I love this! ❤️✨ It is a much better solution, thanks for finding and proposing it @deining 🙏
Edit: oops, it doesn't work as expected :(

We need to drop assets/_vendor. I'll push that in and then merge if the tests still pass.

@chalin
Copy link
Collaborator

chalin commented Feb 1, 2024

Unless I misunderstood, this PR is meant to offer a replacement solution for the files under assets/_vendor, right? As you can see, with these files removed (via 2951dae), the smoke tests are failing: https://github.com/google/docsy/actions/runs/7740838481/job/21106641865?pr=1797. Can you look into it? Thanks!

@deining
Copy link
Collaborator Author

deining commented Feb 1, 2024

Unless I misunderstood, this PR is meant to offer a replacement solution for the files under assets/_vendor, right?

Yes, correct. In an ideal world, we could drop assets/_vendor indeed.

As you can see, with these files removed (via 2951dae), the smoke tests are failing: https://github.com/google/docsy/actions/runs/7740838481/job/21106641865?pr=1797. Can you look into it? Thanks!

But world is not always ideal and (go) software has bugs. The bug that affects us in this case is mentioned here. And the corresponding mount instruction is here:

docsy/hugo.yaml

Lines 45 to 48 in 91efe35

# Mounts for module installations,
# needed to work around a known bug in Go’s module management.
- source: assets/_vendor/bootstrap/scss/
target: assets/vendor/bootstrap/scss/vendor

To make it short, unless we move _rfs.scss to some other place in the repo, we have to keep assets/_vendor. Moving _rfs.scss to another location in the repo seems possible, though, then we have to adjust the mount instruction, too. Afterwards we should be able to delete folder assets/_vendor.

@chalin
Copy link
Collaborator

chalin commented Feb 1, 2024

The whole point of the _cp:bs-rfs NPM script is to ensure that the file is automatically kept fresh. If we can't drop the local copy of _rfs.scss, then the mount points added in this PR are of no use: the UG already builds without those extra mount points even if the _cp:bs-rfs script is removed.

Hmm, maybe #1806 offers a win-win solution. PTAL

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

2 participants