Conversation
Nutomic
requested review from
SleeplessOne1917,
dessalines,
dullbananas and
phiresky
as code owners
dessalines
left a comment
dessalines
left a comment
There was a problem hiding this comment.
This PR doesn't solve the git recompilation issue(the main problem), and still keeps all the version building logic inside lemmy, when it should be moved outside of lemmy as a pre-release step.
I already tried this in another branch, and ran into some docker limitations commented on in the other issue.
If you'd like to do an env var, I'm not opposed, but please keep all the version name logic outside lemmy and only have lemmy read those env vars.
crates/utils/src/lib.rs
Outdated
| None => git_version::git_version!( | ||
| args = ["--tags", "--dirty=-modified"], |
There was a problem hiding this comment.
This is the main issue. Remove this and move it to the docker build (and you'll run into the docker bug I commented on in the other issue). Its causing re-compiles on every git dirty modification.
There was a problem hiding this comment.
Thats not a problem because its only checking the version from git in release builds. Debug builds use the version from Cargo.toml instead. Moving to Dockerfile is not ideal because it should also work for native builds.
crates/utils/src/lib.rs
Outdated
| } | ||
| match option_env!("LEMMY_VERSION") { | ||
| // For nightly add current date | ||
| Some("nightly") => format!("nightly-{}", chrono::Utc::now().date_naive()), |
There was a problem hiding this comment.
Just fill this from docker, its weird to have a special case adding the date when it can just be added in the docker build.
There was a problem hiding this comment.
Did that in the last commit, although I dont like it much because it makes the dockerfile more complex, and needs to be written twice (once for x86 and once for ARM). Doing this in Rust is much easier, I dont understand why you have a problem with that.
crates/utils/src/lib.rs
Outdated
| ) | ||
| .to_string() | ||
| } | ||
| match option_env!("LEMMY_VERSION") { |
There was a problem hiding this comment.
Just make this option_env!("LEMMY_VERSION").unwrap_or(env!("CARGO_PKG_VERSION"))
There's no reason to have special debug cases.
There was a problem hiding this comment.
This is not for debug but for production. Many instances like lemmy.world run their own patches on top of our own releases, and by using the git version like this it is immediately clear. Otherwise they would appear to be running an official release which would be very misleading.
Nutomic
force-pushed
the
version-env-var
branch
from
737d06d to
f2fea1b
Compare
| // Use version from git if none was passed via env | ||
| git_version::git_version!( | ||
| args = ["--tags", "--dirty=-modified"], | ||
| fallback = env!("CARGO_PKG_VERSION") | ||
| ) | ||
| .to_string() |
There was a problem hiding this comment.
I still feel pretty strongly that we should move all this git out of lemmy. Even if it should be run only in release builds, I still think it could affect compiles in weird ways. And something that should be done as a pre-release step anyway. If people want custom versions numbers its on them to set, not us, and it should use the env var (or a version file like lemmy-ui)
I won't block this tho, so merge as you see fit.
|
This is probably unnecessary, and I dont want to make more changes to versioning now that its finally working. |
2 participants
The change in #6246 is overly complicated, it requires extra bash logic to write the version name to a specific file path, and must be formatted to include Rust logic. In comparison this approach is much simpler:
LEMMY_VERSIONenv var which can easily be set via Docker build-arg. If the value isnightlyadd current date, and if it is missing use the current version from git. This avoids cases where instances add their own patches to their forked code, but still show up as running an official release which can make debugging difficult.