starting a makefile (need upstream changes tho)#8
starting a makefile (need upstream changes tho)#8SomethingGeneric wants to merge 4 commits intoalchemicode:Gofrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds build automation tooling for the jServ project, which appears to be migrating from Dart to Go. The changes introduce a Makefile with build, release, and deploy targets, configure Renovate for automated dependency updates, and update .gitignore to exclude build artifacts.
Changes:
- Added Makefile with build, release, and deploy targets for the Go-based jServ application
- Added renovate.json to enable automated dependency updates with automerge
- Updated .gitignore to exclude Go binaries and release bundles (tar.gz, zip)
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Makefile | Introduces build automation with targets for building the Go binary, creating release tarballs, and deploying to system directories |
| renovate.json | Configures Renovate bot for automated dependency updates with automerge enabled |
| .gitignore | Adds patterns to ignore the compiled jServ binary and release archive files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [[ ! -d /etc/jserv ]] && mkdir /etc/jserv | ||
| [[ -f /usr/bin/jServ ]] && rm /usr/bin/jServ | ||
| cp Server/jServ /usr/bin/. | ||
| cp Server/config.json /etc/jserv/. |
There was a problem hiding this comment.
Similar to the release target, copying config.json directly from Server/ to /etc/jserv/ will overwrite any existing production configuration. On subsequent deployments, this could reset customized settings. Consider either skipping the copy if the file exists, or providing clear documentation that users should backup their config before deploying.
| cp Server/config.json /etc/jserv/. | |
| [[ ! -f /etc/jserv/config.json ]] && cp Server/config.json /etc/jserv/. |
| [[ ! -d /etc/jserv ]] && mkdir /etc/jserv | ||
| [[ -f /usr/bin/jServ ]] && rm /usr/bin/jServ |
There was a problem hiding this comment.
The conditional commands will cause make to fail when the condition is false. In make, if any command returns a non-zero exit code, the target fails. When [[ ! -d /etc/jserv ]] evaluates to false (directory exists), the entire command returns false, causing make to stop. Use || true at the end of each line, or rewrite using shell if-statements wrapped in a single shell invocation.
| [[ ! -d /etc/jserv ]] && mkdir /etc/jserv | |
| [[ -f /usr/bin/jServ ]] && rm /usr/bin/jServ | |
| if [ ! -d /etc/jserv ]; then mkdir /etc/jserv; fi | |
| if [ -f /usr/bin/jServ ]; then rm /usr/bin/jServ; fi |
| cd Server && go build | ||
| release: build | ||
| mkdir js-linux | ||
| cp Server/config.json js-linux/. |
There was a problem hiding this comment.
The config.json file may contain sensitive configuration including IP addresses, ports, and permissions settings. Including it in release bundles could leak information about production deployments. Consider documenting that users should customize config.json after installation, or provide a template/example config instead of the actual config file.
| cp Server/config.json js-linux/. | |
| printf '{\n}\n' > js-linux/config.json |
| deploy: build | ||
| [[ ! -d /etc/jserv ]] && mkdir /etc/jserv | ||
| [[ -f /usr/bin/jServ ]] && rm /usr/bin/jServ | ||
| cp Server/jServ /usr/bin/. | ||
| cp Server/config.json /etc/jserv/. |
There was a problem hiding this comment.
The deploy target requires root privileges (writes to /usr/bin and /etc) but doesn't validate permissions or provide clear error messages. The target will fail with cryptic errors if not run with sudo. Consider adding a permission check at the start of the target, or documenting in comments that this target must be run with sudo.
| @@ -0,0 +1,14 @@ | |||
| build: | |||
There was a problem hiding this comment.
Missing .PHONY declarations for the targets. Since build, release, and deploy are not actual files, they should be declared as .PHONY to ensure make always executes them even if files with those names exist. Add .PHONY: build release deploy at the top of the Makefile.
| mkdir js-linux | ||
| cp Server/config.json js-linux/. | ||
| cp Server/jServ js-linux/. | ||
| tar -czvf jserv-linux-amd64.tar.gz js-linux/* | ||
| rm -rf js-linux |
There was a problem hiding this comment.
There's a naming inconsistency: the temporary directory is named 'js-linux' but the output tarball is 'jserv-linux-amd64.tar.gz'. Consider using consistent naming, such as 'jserv-linux' for the temporary directory to match the tarball name pattern.
| mkdir js-linux | |
| cp Server/config.json js-linux/. | |
| cp Server/jServ js-linux/. | |
| tar -czvf jserv-linux-amd64.tar.gz js-linux/* | |
| rm -rf js-linux | |
| mkdir jserv-linux | |
| cp Server/config.json jserv-linux/. | |
| cp Server/jServ jserv-linux/. | |
| tar -czvf jserv-linux-amd64.tar.gz jserv-linux/* | |
| rm -rf jserv-linux |
| release: build | ||
| mkdir js-linux | ||
| cp Server/config.json js-linux/. | ||
| cp Server/jServ js-linux/. | ||
| tar -czvf jserv-linux-amd64.tar.gz js-linux/* | ||
| rm -rf js-linux |
There was a problem hiding this comment.
The release target should check if the build succeeded before proceeding with packaging. If go build fails, the release target will continue and package a potentially non-existent or stale binary. Consider adding error handling or making the build dependency explicit by checking for the binary's existence.
| "automerge": true, | ||
| "packageRules": [ | ||
| { | ||
| "matchUpdateTypes": ["minor", "patch", "digest"], | ||
| "automerge": true | ||
| } | ||
| ] |
There was a problem hiding this comment.
Setting automerge to true at both the root level (line 4) and in packageRules (line 8) is redundant. The root-level automerge enables it for all updates, while the packageRule only enables it for minor, patch, and digest updates. If the intention is to only automerge minor/patch/digest updates, remove the root-level automerge. If the intention is to automerge everything, remove the packageRule as it's redundant.
| "automerge": true, | |
| "packageRules": [ | |
| { | |
| "matchUpdateTypes": ["minor", "patch", "digest"], | |
| "automerge": true | |
| } | |
| ] | |
| "automerge": true |
(maybe leave until you do
--configand add the data stuff to the config, im happy to rebase)