flowey: follow-up to remaining binary size comparison comments #909
Conversation
| @@ -4,6 +4,7 @@ | |||
| //! Gets the merge commit of a PR to base branch | |||
|
|
|||
| use flowey::node::prelude::*; | |||
| use flowey_lib_common::install_git; | |||
There was a problem hiding this comment.
nit: the flowey idiom is to use the full node-path as part of the import and req calls.
There was a problem hiding this comment.
...on that note - you'll want to add a reqv on the install_git's side-effect as part of the emit_rust_step, to make sure that git is actually installed.
(ideally, flowey would warn about unused imports... but that's a tricky problem to solve in the general case)
There was a problem hiding this comment.
Hmm, is it fair to say this node was meant for locally run nodes? Might just be doing something wrong, but actually calling it gives this error on generation:
error while processing flowey_lib_common::install_git: Missing essential request: LocalOnlyAutoInstall
Error: in job 'verify openhcl binary size [x64]': encountered one or more errors
Do GitHub and ADO guarantee that git is available on the machine?
There was a problem hiding this comment.
It's quite possible that this node has not been properly wired up to work in non-local contexts, yes.
Do GitHub and ADO guarantee that git is available on the machine?
I think you should be thinking about this the other way - given that the gh_merge_commit node is not gh/ado specific, and could conceivably be run locally (e.g: if someone wanted to run verify-size locally), it feels like we shouldn't assume git is installed on the machine, no?
of course, the fact we're talking about git muddies the convo a bit (i.e: of course they'll have git installed, who doesn't have git installed?), but the broader point is that when you're writing flowey code, you really want to be doing the best you can to accurately declare the expectations you have of the environment you're running jobs in.
There was a problem hiding this comment.
I went ahead and modified the node so that it should work on CI and locally
flowey/flowey_lib_hvlite/src/_jobs/build_and_publish_openvmm_hcl_baseline.rs
Outdated
Show resolved
Hide resolved
| let auto_install = auto_install.ok_or(anyhow::anyhow!( | ||
| "Missing essential request: LocalOnlyAutoInstall", | ||
| ))?; | ||
| let auto_install = auto_install.unwrap_or(false); |
There was a problem hiding this comment.
Its a little boilerplate-ey at the moment, but look at other instances of LocalOnly requests to see how you should handle the different behaviors on each backend.
i.e: the request should still be required if you run on a local backend
| let artifact_dir = rt.read(artifact_dir); | ||
|
|
||
| let openvmm_openhcl_x86_output = OpenvmmSizeCheckArtifact { | ||
| bin: artifact_dir.join("x64-openhcl-baseline").join("openhcl"), |
There was a problem hiding this comment.
this doesn't seem quite right. at least - it doesn't line up with the format from the publish node above.
hmm, are you sure your artifact download node is returning the right directory? i.e: shouldn't it return the directory directly containing the artifact contents, as opposed to a directory containing another directory, which then contains the artifact contents?
#475 got merged with some outstanding comments. This should clean up the comments that were remaining on that PR.
Included in this change:
dep_onjob for publishing the baseline artifact instead of hijacking thebuild_and_publish_openhcl_igvm_from_recipenodegit_merge_commitfromflowey_lib_commontoflowey_lib_hvliteand renamed togh_merge_commitas this node is somewhat tied to specifically our repo.aarch64binary? We're currently uploading an artifact for it but only diffing thex86_64binary.