fix(Makefile): fix dcspec gen dependencies and hide error output #17043
Conversation
mafredri
force-pushed
the
mafredri/fix-makefile-dcspec-error-output
branch
from
fe5caea to
d915c56
Compare
mafredri
force-pushed
the
mafredri/fix-makefile-dcspec-error-output
branch
2 times, most recently
from
916188f to
6c02c64
Compare
mafredri
force-pushed
the
mafredri/fix-makefile-dcspec-error-output
branch
from
6c02c64 to
cc0f78b
Compare
.github/workflows/ci.yaml
Outdated
| # the target "agent/agentcontainers/dcspec/dcspec_gen.go" depends on | ||
| # node/pnpm tooling. | ||
| mkdir -p node_modules | ||
| touch node_modules/.installed |
There was a problem hiding this comment.
/cc @deansheather lmk if you're unhappy about this change. Happy to solve this another way if you have a suggestion. I'm vary of allowing gen/mark-fresh to touch non-existent files (node_modules/.installed).
There was a problem hiding this comment.
Could you add a new GO_SRC_FILES-adjacent variable that excludes this directory perhaps?
There was a problem hiding this comment.
I can do that 👍🏻. It seems the vpn package is limited to importing codersdk and tailnet (+ sub paths), would it be alright to limit it to these or should we have a more exhaustive selection of src files?
(It'd be nice to eventually partially generate the Makefile and/or use Go src parsing to include relevant src files for targets.)
There was a problem hiding this comment.
I think it would be fine to just exclude the problematic files for now. Even though it only imports those packages, those packages import a lot more.
There was a problem hiding this comment.
Makes sense, wdyt ef77ec5 (#17043)?
mafredri
force-pushed
the
mafredri/fix-makefile-dcspec-error-output
branch
from
ef77ec5 to
ea46eb6
Compare
| @@ -54,6 +54,16 @@ FIND_EXCLUSIONS= \ | |||
| -not \( \( -path '*/.git/*' -o -path './build/*' -o -path './vendor/*' -o -path './.coderv2/*' -o -path '*/node_modules/*' -o -path '*/out/*' -o -path './coderd/apidoc/*' -o -path '*/.next/*' -o -path '*/.terraform/*' \) -prune \) | |||
| # Source files used for make targets, evaluated on use. | |||
| GO_SRC_FILES := $(shell find . $(FIND_EXCLUSIONS) -type f -name '*.go' -not -name '*_test.go') | |||
| # Same as GO_SRC_FILES but excluding certain files that have problematic | |||
| # Makefile dependencies (e.g. pnpm). | |||
| MOST_GO_SRC_FILES := $(shell \ | |||
There was a problem hiding this comment.
To avoid the search again, could you do an inverse grep on GO_SRC_FILES instead perhaps? No stress if you have problems, this will be fine
There was a problem hiding this comment.
AFAIK the := assignment should be evaluated only if used, so only for make build/coder-dylib? If we do an inverse grep then we always have to evaluate GO_SRC_FILES too?
mafredri
force-pushed
the
mafredri/fix-makefile-dcspec-error-output
branch
from
ea46eb6 to
feffa14
Compare
I thought
make genwas failing due to the red output, decided to just hide it. Fixed a few makefile deps while I was at it.