- Notifications
You must be signed in to change notification settings - Fork927
fix(Makefile): fix dcspec gen dependencies and hide error output#17043
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🙏
fe5caea tod915c56Compare916188f to6c02c64Compare6c02c64 tocc0f78bCompare.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
/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 allowinggen/mark-fresh to touch non-existent files (node_modules/.installed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you add a newGO_SRC_FILES-adjacent variable that excludes this directory perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can do that 👍🏻. It seems thevpn package is limited to importingcodersdk andtailnet (+ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense, wdytef77ec5 (#17043)?
ef77ec5 toea46eb6Compare| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AFAIK the:= assignment should be evaluated only if used, so only formake build/coder-dylib? If we do an inverse grep then we always have to evaluateGO_SRC_FILES too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's fair, all g then
ea46eb6 tofeffa14Compare6bf22f8 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.