-
Notifications
You must be signed in to change notification settings - Fork 9
Add a Werkfile for werk itself :)
#44
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
base: main
Are you sure you want to change the base?
Conversation
| let mdbook = which "mdbook" | ||
| let werk-vscode-version = shell "jq --raw-output .version werk-vscode/package.json" | ||
|
|
||
| build "/werk-vscode/werk-{werk-vscode-version}.vsix" { |
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.
What's missing here is running npm install.
I'd love to do
build "werk-vscode/node_modules" {
from "werk-vscode/package-lock.json"
run "npm --prefix werk-vscode install"
}
but that will be rebuilt every time because werk expects the node_modules to be in the target folder. I don't think it supports files in the workspace right now?
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.
Limitations
Separate output directory: It is not possible to put output files next to input files, and files in the output directory are assumed to be generated by werk.
The question is, is this a "not implemented" limitation or "this fundamentally opposes the values of werk" limitation? I think it would be very useful to have but I can understand if you regard this as out of scope.
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.
So a build recipe (currently) can't produce a directory, although that would be an amazing feature.
I think the --prefix flag to npm needs to point into werk's output dir, using a "<...>" interpolated variable. I would like to support the syntax run "npm --prefix <OUT_DIR>/node_modules install", but that requires a few features that are still in the pipeline.
Still, I wouldn't be too worried about integrating NPM with werk's outdatedness tracking, as long as npm gets the right flags. It would be fine (IMO) for something like this to be a task instead of a build recipe, if it can't be supported nicely.
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 would like to support the syntax run "npm --prefix <OUT_DIR>/node_modules install", but that requires a few features that are still in the pipeline.
I think npm --prefix {OUT_DIR} would work but only if npm gets executed from the cwd of vscode-werk. I don't think that's possible right now, right?
But a
build "werk-vscode/node_modules" {
from "werk-vscode/package-lock.json"
cwd "werk-vscode"
run "npm install --prefix {OUT_DIR}"
}would probably not be too hard to add, and also useful because not every tool has a -f / --manifest-path / --prefix flag.
(I also tried run "sh -c 'cd werk-vscode && npm install'" but the quotes don't get parsed: &command_line = Absolute { path: "/usr/bin/sh" } "-c" "\'cd" "werk-vscode" "&&" "npm" "install\'")
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 want sh -c '...' to be supported, feel free to create an issue for it.
I'll much around with npm a bit and try to figure out what the best solution is here. I bet it's a useful thing that people will be looking for in the docs too.
7b2521b to
dd127b6
Compare
3106b47 to
63a33a9
Compare
|
Oooh amazing! I'll take a look at this tomorrow :) |
| } | ||
|
|
||
| task cli-install { | ||
| run "cargo install --path werk-cli" |
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.
Funnily, this will usually clobber the process doing it, which will always fail on Windows. Might be nice for completeness, still.
63a33a9 to
daeae14
Compare
daeae14 to
b7a03f2
Compare
|
I just rebased this on main, it there anything else I should change before this is ready to merge? |
|
Some things discussed in this PR have actually been implemented. If you like, you can go through and bring it up to speed, but I'd also love to have a closer look myself, particularly around the NPM stuff. (Sorry for the delay, I've been a bit pressed for time. 😅) |
no problem, don't feel pressured to look into it, it's not time critical 😄
The main problem (which I think hasn't changed in the Actually, I just noticed that you can easily emulate in-workspace build artifacts by having a build task that creates a dummy file in the target directory and otherwise works in the workspace: build "werk-vscode/node_modules" {
from ["werk-vscode/package.json", "werk-vscode/package-lock.json"]
run ["npm --prefix werk-vscode install", "touch <out>"]
}It actually does exactly what you'd need, if you don't find it too hacky. |
|
Awesome! I think the only hacky thing about it is the naming, because This is a really interesting pattern! Kind of a "reverse .PHONY", in that it does create an output, but the sole purpose of it is to have a node in the dependency graph with a timestamp with some side-effects. You could make this clear by naming the target I'm wondering if this is actually a nice thing to have in general. There could be first-class language support for it, or maybe a recommendation in the book. Also, I'll add |
Dogfooding is always good to find out what features you might want to add. It also adds some discoverability into what this repo contains:
I've also added a
npm run packagescript to werk-vscode, and made it so that keywords are only highlighted when they are at the start of a line, so thattask build {}doesn't highlightbuildas a keyword. I can split that into a separate PR if you want.