-
Notifications
You must be signed in to change notification settings - Fork 34
Support setting version in build.toml
#342
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,6 +163,7 @@ impl General { | |
|
|
||
| super::General { | ||
| name: general.name, | ||
| version: None, | ||
| license: None, | ||
| backends, | ||
| cuda, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -197,12 +197,13 @@ in | |
| "hub" | ||
| "repo-id" | ||
| ] "kernels-community/${buildToml.general.name}" buildToml; | ||
| branch = lib.attrByPath [ "general" "hub" "branch" ] "main" buildToml; | ||
| branch = lib.attrByPath [ "general" "hub" "branch" ] null buildToml; | ||
| branchOpt = lib.optionalString (branch != null) "--branch ${branch}"; | ||
|
Comment on lines
+200
to
+201
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. qq since
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah makes sense! thanks for pointing that out |
||
| # `kernels upload` fails when there are no build variants to upload. | ||
| # However, we do not want this command to error out in that case, so | ||
| # only insert the upload command when there is something to upload. | ||
| uploadStr = lib.optionalString (applicableBuildSets != [ ]) '' | ||
| ${pkgs.python3.pkgs.kernels}/bin/kernels upload --repo-id ${repo_id} --branch ${branch} ${bundle} | ||
| ${pkgs.python3.pkgs.kernels}/bin/kernels upload --repo-id ${repo_id} ${branchOpt} ${bundle} | ||
| ''; | ||
| in | ||
| writeScriptBin "build-and-upload" '' | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly not overly pedantic, I changed it a few times while working on this.
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.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).