Support setting version in build.toml
#342
Conversation
drbh
left a comment
drbh
left a comment
There was a problem hiding this comment.
lgtm, just left a question about the version type
|
|
||
| pub struct General { | ||
| pub name: String, | ||
| pub version: Option<usize>, |
There was a problem hiding this comment.
likely overly pedantic, but maybe we want to be more strict on the version and disallow negative numbers or 0? additionally maybe we want to use a less ambiguous type like u32.
conversely maybe we want a more open ended type like a string? just thinking out loud here..
curious to hear your thoughts on the type?
There was a problem hiding this comment.
Certainly not overly pedantic, I changed it a few times while working on this.
but maybe we want to be more strict on the version and disallow negative numbers or 0?
Negative numbers are already disallowed, >= 0 I feel adds the unnecessary complication of having to use NonZero. I think it's also a nice number to show that the kernel is still in the initial stages.
conversely maybe we want a more open ended type like a string?
I find this a hard one. Initially I had a string and some additional validation to check that the string can be parsed as usize. When it was still called a channel, my idea was that you could also make testing and development channels and you would put something like "foo-test" here, it would get uploaded to the channel channel-foo-test.
The primary objections I can think of are: making it a string could confuse people into thinking they could put a full dotted version there and it makes you have to do more parsing and validation. But the objections are not particularly strong.
That said, I think we could easily transition to a string in the future by either changing the type to an enum or by upgrading to version v4 of the configuration. Using an enum could even be semantically clearer: either it is a version or it is something else (that we haven't thought of yet).
MekkCyber
left a comment
MekkCyber
left a comment
There was a problem hiding this comment.
Thank you! I just have few questions
| branch = lib.attrByPath [ "general" "hub" "branch" ] null buildToml; | ||
| branchOpt = lib.optionalString (branch != null) "--branch ${branch}"; |
There was a problem hiding this comment.
qq since lib.attrByPath [ "general" "hub" "branch" ] is an integer, for example 1, i'm not sure why this gets uploaded to v1, instead of just 1 ?
There was a problem hiding this comment.
general.hub.branch is still a stringly-typed branch. If it is not set (the normal case), then the --branch option is not passed, so it will be a regular kernels upload repo_id. In that case, the version upload code in kernels upload will be used:
There was a problem hiding this comment.
ah makes sense! thanks for pointing that out
lib/gen-flake-outputs.nix
Outdated
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| ${bestBuildSet.pkgs.python3.pkgs.kernels}/bin/kernels upload --repo-id ${repo_id} --branch ${branch} ${bundle} | ||
| ${bestBuildSet.pkgs.python3.pkgs.kernels}/bin/kernels upload --repo-id ${repo_id} ${branchOpt} ${bundle} |
There was a problem hiding this comment.
If the version isn’t set, I’m assuming we’re uploading to main.
I’m not sure whether it’s better to automatically infer the version when the user doesn’t specify it, or to simply raise a warning reminding them to set a version explicitly.
There was a problem hiding this comment.
Yes, in that case we are uploading in main. I am not sure about setting it for the user automatically, I think it should be opt-in, because initially a user will want to upload both to a version branch and to main (for backwards compatibility) up till when the first version bump happens.
We should indeed emit a warning in build2toml when the version is not set. But I think we should only add it once versions are supported everywhere (once we have a versions-enabled kernels release).
There was a problem hiding this comment.
Yes I agree! let's do that
This change adds the `version` option to the `general` section in `build.toml`. The version will be written to the build metadata and will be used by `kernels upload` to upload builds to a versioned branch.
This allows version-based uploading to kick in when no branch is specified.
danieldk
force-pushed
the
version-option
branch
from
0f5b327 to
0798703
Compare
This change adds the
versionoption to thegeneralsection inbuild.toml. The version will be written to the build metadata and will be used bykernels uploadto upload builds to a versioned branch.