-
Notifications
You must be signed in to change notification settings - Fork 22
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
Generate single-file packages with the docs #20
Conversation
This PR will create two new single-file packages of the library: `au.hh`, and `au_noio.hh`. They will be accessible at these locations in the built doc website. To test, run `tools/bin/au-docs-serve`, and navigate to the following URLs: - http://127.0.0.1:8000/au/au.hh - http://127.0.0.1:8000/au/au_noio.hh
BUILD
Outdated
name = "single_file_au", | ||
srcs = ["//au:headers"], | ||
outs = ["docs/au.hh"], | ||
cmd = "$(location tools/bin/make-single-file) au/au.hh au/io.hh > $@", |
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 should be able to simplify this as:
cmd = "$(location tools/bin/make-single-file) au/au.hh au/io.hh > $@", | |
cmd = "$(location tools/bin/make-single-file) $(SRCS) > $@", |
Same below.
Separately, I find $(OUTS)
more readable than $@
, but I'll leave that up to you.
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, I see. You have a big glob as a dependency, then down-select inside the cmd
. That works, but is sub optimal. It'd be better to select the inputs you actually care about in srcs
, then use all of them in the cmd
itself.
So roughly like:
genrule(
name = "single_file_au",
srcs = [
"//au:au.hh",
"//au:io.hh",
],
outs = ["docs/au.hh"],
cmd = "$(location tools/bin/make-single-file) $(SRCS) > $@",
tools = ["tools/bin/make-single-file"],
)
Otherwise, you always build single_file_au_noio
even if you're only changing the io.hh
file.
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 didn't know about $(OUTS)
; thanks! I was just adapting from the examples I had found.
As for $(SRCS)
, I hadn't been able to get this to work at first, because I didn't know about exports_files()
. I got that part to work, but unfortunately, what this script is doing is intrinsically somewhat messy. It needs to actually open every header which is transitively included (so it can combine their contents into a single file). This means we have two options as I see it:
- Manually reproduce the include graph in the
srcs
file here. - Depend on all headers, and let the script decide which ones to actually pull in.
Much as I typically hate over-broad dependencies, I think the maintenance cost of the first option would actually be worse. Of course, I'm always open to suggestions if you see a third way!
srcs = ["//au:headers"], | ||
outs = ["docs/au.hh"], | ||
cmd = "$(location tools/bin/make-single-file) au/au.hh au/io.hh > $@", | ||
tools = ["tools/bin/make-single-file"], |
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 to admit that I'm not super thrilled that this is using the host Python here. Why not use the one we grabbed from rules_python?
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 definitely feel the same! The background is that I wrote that script before we had a hermetic python (in fact, even before I knew that we were likely to get it). Once I saw what was possible, I wanted to update this script to take advantage of it. I've filed #21 to track this.
genrule( | ||
name = "single_file_au", | ||
srcs = ["//au:headers"], | ||
outs = ["docs/au.hh"], |
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.
On a separate, unrelated note: The .hh
extension is an undocumented Aurora-ism. The Google style guide dictates .h
for the extension.
header files should end in .h.
Might be worth adding a style guide section to the README at some point that documents our deviations from the Google style guide.
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.
Good catch: filed #22 to track.
This PR will create two new single-file packages of the library:
au.hh
, andau_noio.hh
. They will be accessible at these locationsin the built doc website.
To test, run
tools/bin/au-docs-serve
, and navigate to the followingURLs:
Later on, when we make the installation doc, we will link directly to
these files. We'll also want to make some tests for these generated
files.