-
Notifications
You must be signed in to change notification settings - Fork 233
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
Emit a fileset-grouped manifest from Golden Gate #1117
base: main
Are you sure you want to change the base?
Conversation
fc29bd9
to
7aa9235
Compare
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.
Overall, I think it is an improvement to have GG explicitly enumerate the optional artifacts that it is creating so that the makefile read that manifest and use it to do the right thing rather than having hardcoded artifact lists that are far removed from GG. I also think it is a good idea to use a build-tool-agnostic representation of the manifest. JSON is fine.
However, I think the current implementation isn't correct because it won't copy files that are added into the manifest by any GoldenGate run done by the current make invocation. I think the solution to this is to:
- create a tool that reads your JSON and spits out a generated makefrag representation of it that you explicitly
include
(note the intentional use ofinclude
instead of-include
) - Add an explicit dependence from the makefrag to the manifest to the inputs of GoldenGate
- Rethink how Distributed Elaboration works because 1&2 is definitely doing to break the 'cute' introspection of the
replace-rtl.sh
script I send to the distributed builders. One possiblity might be to add an additional cmdline argument like--gen-manifest-only
to GoldenGate where it doesn't actually run and generate the 'real' outputs because those are computationally expensive (more like, my worst nightmare), but it does generate the manifest correctly (because hopefully that should be easy by comparison).
|
||
val fileManifestSuffix = ".file-manifest.json" | ||
val emittedVerilogKey = "EmittedVerilogFiles" | ||
val unclassifiedKey = "UnclassifiedFiles" |
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.
Is it a good idea to comment on the behavior of the unclassifiedKey
here? Without RTC, I'm guessing that those files will go to all downstream flows? 🤷
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.
Yeah, you point to this object from other places to say 'go look here', you need to explain what is expected for unclassifiedKey
.
# Assembles strings that can be used in $(shell <query>) commands to look up | ||
# filesets from GG's output manifest. These are strings so that can be emitted | ||
# literally into a script to run on a remote as part of distributed elaboration. |
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.
Aww, how thoughtful... you actually tried not to break my unmerged and unloved feature even more than it has been already. 😍
cp -f $< $@ | ||
|
||
# Goes as far as setting up the build directory without running the cad job | ||
# Used by the manager before passing a build to a remote machine | ||
replace-rtl: $(fpga_delivery_files) $(fpga_sim_delivery_files) | ||
replace-rtl: $(fpga_delivery_files) $(fpga_sim_delivery_files) $(fpga_work_dir)/stamp | ||
if [[ "$(firstword $(fpga_delivery_files))" != "$<" ]]; then \ |
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.
I need a decoder ring to understand what this conditional is doing, when is $(firstword $(fpga_delivery_files))
not going to be $<
? I thought that $<
is the first prerequisite.
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.
You're doing something special here and handling the case where $(fpga_delivery_files)
is the emptystring because you haven't run before and the manifest doesn't exist yet, so $<
will be stamp
... probably worth a comment at least.
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.
Why would you not always defer evaluation of the manifest until the downstream target recipe is executing? I think it would guard against the case where the manifest is updated by rerunning GG to contain more or less files.
To be more verbose, I'm concerned about:
- make starts with manifest in existence
- when
replace-rtl:
target is defined (during reading of the makefiles),$(fpga_delivery_files)
has contents defined by the then-existing manifest. - For one of many reasons, GoldenGate needs to be rerun and it changes the manifest to include more stuff.
- Your recipe here in
replace-rtl:
won't copy the 'more stuff' in the manifest.
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.
If you're worried about unnecessary copying causing the timestamps to be updated in $(fpga_whatever_dir)
, you could use rsync
instead of cp
since it will use the files hash to determine whether to really copy it...
# Assembles strings that can be used in $(shell <query>) commands to look up | ||
# filesets from GG's output manifest. These are strings so that can be emitted | ||
# literally into a script to run on a remote as part of distributed elaboration. | ||
manifest_query = cat $(gg_file_manifest) 2> /dev/null | jq -r '.$(1) | .[]' |
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.
Why not just:
manifest_query = jq -r '.$(1) | .[]' $(gg_file_manifest)
?
Oh, nevermind, I see what happens, jq
just sits there if $(gg_file_manifest)
is empty string because no arg gets passed to jq
and it sits spinning on STDIN... What if you put quotes around '$(gg_file_manifest)'
does it avoid that problem and make it slightly easier to read?
An alternative that I will agree is not easier to read would be to put another make function call in your function because that's when $(gg_file_manifest) has to be defined and make it slightly more obvious to users when the makefile logic is broken:
manifest_query = cat $(gg_file_manifest) 2> /dev/null | jq -r '.$(1) | .[]' | |
manifest_query = $(if $(gg_file_manifest), jq -r '.$(1) | .[]' '$(gg_file_manifest)', $(error gg_file_manifest must be defined)) |
NOTE: my call to $(if)
is using the documented gnumake convention that any non-empty string evaluates to true
This should make it more clear what files are used where, allowing me to clean up some of the
replace-rtl
logic (we can drop per-platform junk), and easing integration into other build systems.This output file manifest is rooted at
TargetDir
, all listed paths are relative to that directory. Note this does not include verilog blackboxes, and the.f
. Collecting those files and including them here is going to be a chunk of work that might require duplicating a bunch of FIRRTL code since black box file emission is done ad-hoc does not reuse the CustomFileEmission machinery.Here's an example emission:
Some of the scala names are a little jank, i think
Fileset
is a better, shorter description of what i was trying to capture with DownstreamFlow / Dependency.Related PRs / Issues
UI / API Impact
Verilog / AGFI Compatibility
Contributor Checklist
changelog:<topic>
label?ci:fpga-deploy
label?Please Backport
label?Reviewer Checklist (only modified by reviewer)
changelog:<topic>
label?