gen-device: only generate device files for supported devices#5456
gen-device: only generate device files for supported devices#5456niaow wants to merge 2 commits into
Conversation
This changes the make gen-device command such that it only converts relevant device files. This saves a lot of build time. The process of determining which files to convert is now handled by make. Incremental builds are now supported. Appropriate parallel building is now done for all device generation (previously it was only handled properly for AVR). The generated file names were updated to match their source name formatting. I could probbably lowercase everything when converting them, but this would complicate the makefile. I don't really think this is worth it.
|
Before: After: This is much faster and it reduces the storage used by device files by 86%. |
| } | ||
|
|
||
| func formatText(text string) string { | ||
| text = regexp.MustCompile(`[ \t\n]+`).ReplaceAllString(text, " ") // Collapse whitespace (like in HTML) |
There was a problem hiding this comment.
Might not be in scope of this actual PR but I stumbled across this some time ago - this regex is unnecessarily compiled multiple times
dgryski
left a comment
dgryski
left a comment
There was a problem hiding this comment.
Seems reasonable, although I'll leave it to someone who actually deals with this files for the final word.
I think we should probably preserve the lowercase file names if we can to avoid case issues on OS like Windows. |
I am not entirely sure what you mean by this. I understand that Windows is case-insensitive, but we are just matching the source case here? |
|
I made a patch that changes them back to lowercase. It is much more complicated in the makefile since we cannot just use pattern rules (which expect the source and destination name stems to match). I had to shell out to |
|
I was testing a little and now I see what you meant. I also am not sure if the case thing is worth it, since all of the real work happens due to either build tags or the target JSON file anyhow. Good that you made it a separate commit, let me consider my opinion now. BTW the speed of it is fantastic. We will also have to think about how to modify the instructions for adding a new board, since you will have to add to the makefile to get it generated. |
|
Question: is the primary goal 1) to reduce build time, or 2) to reduce the size of the directory? Asking since, in the case of 1) a much more effective approach might be to generate these files once and put them in a submodule so they don't need to be generated every time. We already have a few submodules, adding another one wouldn't be too much trouble. In the case of 2), I think moving them to a submodule would in fact still be better since we can avoid downloading a few other big submodules like lib/cmsis-svd and lib/stm32-svd. Downloading all these submodules also takes a few minutes in CI, so it would be very beneficial to not have to do that every build. |
|
My intent was sorta both, although the "size" I cared about was the size of the final release as opposed to the size of the files passed to the build. As such, generating only the files for the supported devices (74 MiB) as opposed for all of the chips (514 MiB) is beneficial to me. I can see the benefit to moving the generation outside of the main repo and importing it, although it is important to note that this will add an extra step every time we want to change something. Long-term, I wonder whether it would be better to import a package that is vendored by the goroot (like upstream does with |
aykevl
left a comment
aykevl
left a comment
There was a problem hiding this comment.
I think this is a good idea. I would personally prefer if the 2nd commit was removed, using the original source names seems like a good idea and is a change I was considering a while ago anyway.
My intent was sorta both, although the "size" I cared about was the size of the final release as opposed to the size of the files passed to the build.
Yeah that's entirely reasonable. And in fact, we can move these files later to a submodule if we want to (the benefits are more or less ortogonal).
| // Format the source. | ||
| formatted, err := format.Source(buf.Bytes()) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Technically a separate change, but it's a good idea regardless.
| ifneq ($(RENESAS), 0) | ||
| gen-device: gen-device-renesas | ||
| # Renesas is currently unused | ||
| #gen-device: gen-device-renesas | ||
| endif |
There was a problem hiding this comment.
Might be worth removing entirely (possibly in a separate PR), including the submodule. It's currently just dead weight.
5 participants
This changes the make gen-device command such that it only converts relevant device files. This saves a lot of build time.
The process of determining which files to convert is now handled by make. Incremental builds are now supported.
Appropriate parallel building is now done for all device generation (previously it was only handled properly for AVR).
The generated file names were updated to match their source name formatting. I could probbably lowercase everything when converting them, but this would complicate the makefile. I don't really think this is worth it.