-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Wit/Wake use documentation #2111
Conversation
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 have a few suggestions for improvements, but this otherwise looks great to me!
|
||
Because the constructor is private, we use `makeRocketChipGeneratorOptions` to construct `RocketChipGeneratorOptions`: | ||
|
||
* `jars` is a List of compiled Scala jars for your project, for example `rocketchipScalaModule.scalaModuleClasspath` for base Rocket Chip. |
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.
Since it isn't completely obvious to me, let me double check my understanding. Are these are meant to include both the rocket-chip dependencies as well as the actual rocket-chip application? And to double check, in this flow we do not create a fat JAR, right? We may want to make that explicit here (also maybe as a comment in the actual tuple definition in the code).
Also, technical writing nit: JAR should be spelled in all caps (in your prose).
Other nit: Shouldn't jars
be Jars
? Wake tuple fields are spelled with an uppercase initial letter, and for people who are less familiar with Wake, you really should match the case or else it could cause confusion because it may look like jars
is not the same thing as Jars
.
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.
Yes it includes rocket-chip's dependencies and rocket-chip itself, everything as JARs.
We do not create a fat JAR, I think that does belong in the documentation of API Scala SiFIve, and yeah this tuple should mention it needs all of the necessary compiled JARs. Technically this code is agnostic as to where the JARs come from, someone could pass a fat JAR if they wanted to.
In those bullets, I'm referring to the arguments of makeRocketChipGeneratorOptions
, I can clarify that or also include which parts of the tuple they correspond to, what do you think?
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.
We do not create a fat JAR, I think that does belong in the documentation of API Scala SiFIve, and yeah this tuple should mention it needs all of the necessary compiled JARs. Technically this code is agnostic as to where the JARs come from, someone could pass a fat JAR if they wanted to.
Even if API Scala SiFive does not create fat JARs, it wasn't completely obvious to me when reading just this documentation whether this specific function takes in just the JAR of the application or the JARs of the application and all of its dependencies.
In those bullets, I'm referring to the arguments of makeRocketChipGeneratorOptions, I can clarify that or also include which parts of the tuple they correspond to, what do you think?
My bad, I got confused while reviewing your text and thought you were describing the Tuple itself. Maybe I was just expecting you to document the Tuple itself, since the Tuple is defined before the constructor function, and that's why I got confused.
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'll try to make the prose more clear 🙂
|
||
* `jars` is a List of compiled Scala jars for your project, for example `rocketchipScalaModule.scalaModuleClasspath` for base Rocket Chip. | ||
* `targetDir` is the directory where the output of the generator will be placed. | ||
* `topModule` is the name of the top Chisel Module, eg. `"freechips.rocketchip.system.TestHarness"` |
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.
Might be good to replace "name" with "fully-qualified name".
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.
Agreed
You can run the generator with the Wake rule `runRocketChipGenerator`. | ||
This function accepts a single `RocketChipGeneratorOptions` as an argument. | ||
|
||
---- |
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.
Although GitHub doesn't have a built-in Wake syntax highlighter, if we ever produce static HTML versions of these READMEs it'll be nicer to explicitly annotate the code block with the language so that we can apply the appropriate syntax highlighting.
See how I did this in the Wake documentation that I wrote recently: https://github.com/sifive/wake/blame/2dba999205b58ace3dc7d92a1010335f201dff58/share/doc/wake/from-scala.adoc#L40
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.
Oh good call, I'll add that
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 do think we should look into generating websites sooner rather than later, easier than trying to get Github to accept Wake as a language and then we can use the more advanced features (like pulling in code rather than copy pasting it which I am only currently doing out of necessity.
|
||
* You can see the job that created a file with `wake -o <file>`, and jobs that use a file as input with `wake -i <file>` | ||
|
||
* You can combine `-o` and `-i` with `-s` to create an exectuable bash script that runs a job. |
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.
Might be worth mentioning that the executable shell script includes things like setting up all the environment variables.
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.
Agreed
|
||
* You can combine `-o` and `-i` with `-s` to create an exectuable bash script that runs a job. | ||
|
||
* By default, Wake does not maintain a stack trace, use `-d|--debug` to have Wake create one. |
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.
* By default, Wake does not maintain a stack trace, use `-d|--debug` to have Wake create one. | |
* By default, Wake does not maintain a stack trace; use `-d|--debug` to have Wake create one. |
wake has been removed in #2847 |
Squash and merge please!
This is just a start, but it's something.
Related issue:
Type of change: other enhancement
Impact: no functional change
Development Phase: implementation
Release Notes
Add initial documentation for Wit and Wake flow.