Use standard directories when running on Linux #91
Conversation
There was a problem hiding this comment.
I just took a quick look for now
General suggestions:
NixOS-specific suggestions:
Nixos tries really hard to build packages in an isolated and reproducible manner. Because of this, we have a wrapper for cargo that downloads and checksums the dependencies separately, so it'd be great to have separate targets to build/install the binary and to install the data files (nixos would only use the latter).
Something like this:
.PHONY: all build install install-bin install-data clean uninstall
[...set the variables...]
build:
cargo build --release
install: install-bin install-data
install-bin: build
install -Dm755 ./target/release/polaris $(BINDIR)/polaris
install-data:
install -d $(DATADIR)
cp -r ./web $(DATADIR)/web
cp -r ./swagger $(DATADIR)/swagger
There was a problem hiding this comment.
Just a couple suggestions for now.
i'm still testing this, but I think there'll also be some regressions:
- Not being able to run the installed program as a normal user, since as far as I can see there's no way to override the data/cache/log directory
- Not being able to run the tests after running
make(not that important)
res/unix/Makefile
Outdated
| BINDIR ?= /usr/local/bin | ||
| DATADIR ?= /usr/local/share |
There was a problem hiding this comment.
| BINDIR ?= /usr/local/bin | |
| DATADIR ?= /usr/local/share | |
| PREFIX ?= /usr/local | |
| BINDIR ?= $(PREFIX)/bin | |
| DATADIR ?= $(PREFIX)/share |
There was a problem hiding this comment.
I just pushed some follow-up changes. Your suggestion lead me to following the Make manual more closely. The data/cache/log directories are now also under /usr/local by default, but can be overridden by setting LOCALSTATEDIR.
Tests should work regardless of all this as they do not rely on any of these values (with the caveat they only validate the software, not its installation).
res/unix/Makefile
Outdated
| rm -r $(POLARIS_BIN_DIR) | ||
| rm -r $(POLARIS_DATA_DIR) | ||
| rm $(POLARIS_BIN_PATH) | ||
| rm -r $(POLARIS_WEB_DIR) | ||
| rm -r $(POLARIS_SWAGGER_DIR) | ||
| rm -r $(POLARIS_DB_DIR) | ||
| rm -r $(POLARIS_LOG_DIR) | ||
| rm -r $(POLARIS_CACHE_DIR) | ||
| rm -r $(POLARIS_PID_DIR) |
There was a problem hiding this comment.
I think data files shouldn't be removed on uninstall (most programs leave them there)
There was a problem hiding this comment.
I can't find any official recommendation on this, but I think as a user I would prefer everything gone from my system. I will leave this as-is unless there is a strong argument for leaving files behind.
|
Sorry, I've been a bit busy. I'll give this some proper testing next week |
|
No worries! |
Or next month ;) |
Co-authored-by: Francesco Gazzetta <fgaz@fgaz.me>
src/main.rs
Outdated
| options.optopt("", "log", "set the path to the log file", "FILE"); | ||
| options.optopt("", "pid", "set the path to the pid file", "FILE"); | ||
| options.optopt( | ||
| "l", | ||
| "log", |
There was a problem hiding this comment.
Good catch. This PR is already fat breaking changes so I think I'll rename the old log option to log-level.
|
I just tested again, and found a problem with the
then the default data paths are going to be relative to the current directory. This is because the installed executable is the one that as built with |
|
Wow, I would 100% have missed that and shipped it. Thanks again for your help with this work. I fixed this issue by splitting the |
|
@fgaz Would you have time to give this another try before I merge it for good? |
|
@agersant I just did a quick test, and it seems to work great! |
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
- Coverage 70.27% 70.11% -0.17%
==========================================
Files 22 22
Lines 1440 1442 +2
==========================================
- Hits 1012 1011 -1
- Misses 428 431 +3
Continue to review full report at Codecov.
|
Proposed fix for #39