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

Outsource startup call to script and document building approach #68

Closed
wants to merge 6 commits into from

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Nov 26, 2023

fix #7

@pat-s pat-s requested a review from anbraten November 26, 2023 16:09
Comment on lines +1 to +14
.PHONY: all
all: build test

.PHONY:
build:
corepack enable
pnpm build

.PHONY: test
test:
pnpm test

.PHONY: clean
clean:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? Can't this be done completely from pnpm scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use it for local dev purposes, and having a generic way of building and testing is what make commands are all about. What is the issue with these?

Copy link
Contributor

@qwerty287 qwerty287 Dec 11, 2023

Choose a reason for hiding this comment

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

It adds more code/files without an important usecase.
E.g. for woodpecker we also just use pnpm scripts for building/linting/etc the web ui.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without an important usecase.

Hm, this is certainly a subjective view/topic. When does an "important usecase" start WRT to makefile rules? Two lines? When a "cd" is involved? There are also a few "simple" makefile rules in the main repo which should be removed then.

I am also not sure that I agree that a Makefile or multiple small Makefile rules add "bloat" to a repo.
Makefiles become helpful when devs try out something small and their are not super deep into the language and just want to invoke what's needed to install the deps and invoke tests etc. E.g. I am not a node person and there are so many frameworks one can use and having makefile rules for installing deps and running tests is worth a lot in these cases (just as one example).
In addition, makefiles function as an implicit contributing documentation, i.e. one can just look up the rules and infer easily how to get started (install deps, build, etc).

No big deal about this particular PR but if the file/rules are not wanted here, this should be applied org-wide and we should document what is required to get a makefile rule added to a repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

No big deal about this particular PR but if the file/rules are not wanted here, this should be applied org-wide and we should document what is required to get a makefile rule added to a repo.

In general, I don't think they're bad, but pnpm has this natively already, why not use it? For go commands or similar I think it's a good way (besides the issues make has like the unnecessary complex syntax etc).

However, I don't think you should use undocumented makefiles as a source about how to build etc. Instead, I'd add instructions to the readme which make this clear.

If this makefile would include as many rules as e.g. the main repo (which also has a "help" screen), I'd be totally fine with adding it. But having make rules running a single command is just not necessary in my opinion.

@pat-s pat-s closed this Aug 5, 2024
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.

Simplify CMD in docker image?
3 participants