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

Fall back to trying ex.New(binaryName, cmdArgs...) when ex.Npx fails for PostCSS etc. #12486

Closed
kurotych opened this issue May 12, 2024 · 6 comments · Fixed by #12616
Closed
Assignees
Milestone

Comments

@kurotych
Copy link

kurotych commented May 12, 2024

I want to use a theme (https://github.com/google/docsy) but I don't want to install any NodeJS garbage on my host machine.

As a workaround, I created a Dockerfile, wrapped script and rebuilt hugo

FROM node:20-alpine

WORKDIR /usr/src/app

RUN npm install -g postcss postcss-cli autoprefixer

CMD [""]
docker build -t postcss .

❯ cat postcss

#!/bin/bash
docker run --rm -i postcss postcss "$@"

Rebuild Hugo with a changed next-line because Npx function adding the npx prefix that I don't need.

cmd, err := ex.Npx(binaryName, cmdArgs...)

=>

ex.New(binaryName, cmdArgs...)

It'd be nice to have a configurable binary path to the postcss to make it possible to call without npx prefix.

@almereyda
Copy link

almereyda commented Jun 6, 2024

I'm experiencing a similar drawback when working from within this dev container.

{
  "name": "Build environment",
  "image": "mcr.microsoft.com/devcontainers/base:bullseye",
  "features": {
  "ghcr.io/devcontainers/features/git:1": {},
    "ghcr.io/devcontainers/features/go:1": {},
    "ghcr.io/devcontainers/features/hugo:1": {
      "extended": true
    },
    "ghcr.io/devcontainers/features/node:1": {}
  }
}

postCSS filters are not applied and main.scss does not build a main.css file.

@bep bep modified the milestones: v0.127.0, v0.128.0 Jun 8, 2024
@cjshearer
Copy link

cjshearer commented Jun 21, 2024

I ran into this myself. I can't use the postcss-cli package from nixpkgs, because resources.PostCSS shells out to npx to run postcss. An easy fix would be to make hugo's calls for npx binaries fall back to checking the user's PATH for binaries by the same name - it would be backwards compatible and afaik would follow hugo's security model.

An aside:
I'm curious what @bep's plan is for the new tailwind oxide engine. The only reason I have a package.json with

  "devDependencies": {
    "@fullhuman/postcss-purgecss": "^5.0.0",
    "@tailwindcss/typography": "^0.5.13",
    "autoprefixer": "^10.4.19",
    "postcss": "^8.4.38",
    "postcss-cli": "^11.0.0",
    "postcss-html": "^1.7.0",
    "tailwindcss": "^3.4.4"
  },

is because of tailwind. Once we get the new tailwind engine, I expect it will warrant its own resource_transformer. I realize @kurotych, @almereyda, and I are the 1% of the 5% who care, but I'm hoping it won't depend on npx; I could ditch the whole npm/nodejs toolchain.

Related discussion:

@bep
Copy link
Member

bep commented Jun 21, 2024

I'm curious what @bep's plan is for the new tailwind oxide engine.

Odd timing, I was just testing Tailwind Oxide:

bep/hugo-starter-tailwind-basic#19

It's convenient to have npm install and maintain the version of this stuff, but if we could get rid of PostCSS for TailwindCSS, that would be pretty great. A first glance it looks like the Tailwind CLI lacks some options in this area, but that could be fixed.

As to this issue:

It'd be nice to have a configurable binary path to the postcss to make it possible to call without npx prefix.

Hugo currently only executes predefined binary names that's in the OS PATH. Having a way to execute arbitrary binaries (even if that could be controlled) is not something I'm opening up for. We could/should probably have a useNpx or something, but I'm not sure where to put it.

@bep
Copy link
Member

bep commented Jun 21, 2024

❯ cat postcss

Why not create a npx wrapper that do similar?

@cjshearer
Copy link

I don't want to speak for @kurotych, but given that they are already building and running a docker container with a postcss shellscript, adding an npx wrapper doesn't seem like much of an additional lift.

However, I think both of our problems would be solved easily and securely by falling back to checking for a binary of the same name using ex.New(binaryName, cmdArgs...), when the following fails:

cmd, err := ex.Npx(binaryName, cmdArgs...)
if err != nil {
if hexec.IsNotFound(err) {
// This may be on a CI server etc. Will fall back to pre-built assets.
return &herrors.FeatureNotAvailableError{Cause: err}
}
return err
}

Basically:

  1. check if postcss exists with npx (already done)
  2. failing that, check if it is installed globally

@bep
Copy link
Member

bep commented Jun 21, 2024

However, I think both of our problems would be solved easily and securely by falling back to checking for a binary of the same name using ex.New(binaryName, cmdArgs...)

I agree. I'm going to repurpose this issue some something that's both simple and actionable, and if that does not fix it, we'll revisit later.

@bep bep changed the title Running postcss in docker Fall back to trying ex.New(binaryName, cmdArgs...) when ex.Npx fails for PostCSS etc. Jun 21, 2024
@bep bep added Enhancement and removed Proposal labels Jun 21, 2024
@bep bep self-assigned this Jun 21, 2024
@bep bep modified the milestones: v0.128.0, v0.129.0 Jun 21, 2024
bep added a commit to bep/hugo that referenced this issue Jun 22, 2024
bep added a commit to bep/hugo that referenced this issue Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants