New formula internal json v3 dependencies format #17153
Conversation
apainintheneck
changed the title
This improves the readability of dependencies in the json we produce for this internally.
apainintheneck
force-pushed
the
new-formula-json-dependencies-format
branch
from
32ee8ae to
541305a
Compare
Library/Homebrew/test/support/fixtures/internal_tap_json/homebrew-core.json
Show resolved
Hide resolved
apainintheneck
changed the title
Might be nice to verify this by manually generating the formulae.brew.sh API output and diffing them? |
There was a problem hiding this comment.
Looks good. All comments non-blocking. Happy to merge and iterate further. Thanks @apainintheneck!
Library/Homebrew/formula.rb
Outdated
| return if spec_symbol != :stable && spec_symbol != :head | ||
|
|
||
| send(spec_symbol)&.declared_deps&.each_with_object({}) do |dep, dep_hash| | ||
| next if dep.implicit? # Remove all implicit deps |
There was a problem hiding this comment.
Would be good to say why they are being removed rather than that they are
There was a problem hiding this comment.
This comment was copied from the old logic. I'll do some digging and update it.
There was a problem hiding this comment.
It seems like implicit dependencies are really only needed when building from source and are used when downloading and unpacking files. See DependencyCollector for more details.
- rename #dependencies_list to #internal_dependencies_hash - the initial implementation returned an array but now it doesn't - simplify usage of #tap in #internal_dependencies_hash - remove safe navigation operator usages in #internal_dependencies_hash - better document why implicit dependencies are not included in the API JSON - add new test fixture formula to better test generation of uses from macos bounds with the new internal json format
|
I'm hoping that I've addressed all of the feedback in 6ad02b8. Let me know if there's anything I've missed. It adds a new test for a formula with uses from macOS bounds, cleans up syntax and adds some more documentation. |
Yeah, I ran some extra tests locally and everything looks good to me. I'm not too worried about the generation side of things since that's relatively easy to test. The loading side is a bit more difficult but I didn't really touch anything that should affect the JSON v2 loading logic and those things are covered by some tests too. It was more just to let everyone know in case something goes haywire. $ brew api-readall-test --formula
Generating formulae API ... and mocking formula API loader.
Read core formula and saw no failures!
$ brew generate-api-diff --formula --stat
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Switched to branch 'new-formula-json-dependencies-format'
.../api/internal/v3/homebrew-core.json | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)Note: It makes sense that the internal JSON v3 file is different after this change but everything else looks to be identical. See my repo with dev commands for more info about each of them: https://github.com/apainintheneck/homebrew-dev-utils |
|
Thanks @apainintheneck! |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?See #16410 (comment)
This implements a new way of storing dependencies in the internal formula JSON v3 format. The goal is that this format is easier to understand at a glance, easier to generate and easier to parse. I think with this PR two of those three goals have been achieved.
The parsing code ended up being a similar level of complexity to what we saw before. In the end, the JSON output will be slightly larger as mentioned in the thread linked above but the overall JSON size will still be much smaller than before. We're talking about half a megabyte difference when the total savings will end up being around 11 megabytes out of what was originally a 24 megabyte file (I haven't double-checked these numbers but they should be in the neighborhood and more details can be found in the thread).
The generation code is much simpler than before. Take a look at the old
Formula#dependencies_hashmethod and the newFormula#dependencies_listmethod.The biggest risk here is that it affects JSON generation for the public formula JSON v2 format or that the current JSON parsing logic doesn't work the same way it did before. If there are any problems with the internal JSON v3 parsing, no one's using it right now so it's not a big deal.