Move part of autobuild() into new get_meta_json() helper #872
Conversation
|
I'm generally in favor of this change, but I'll let @giordano have final approval. |
The two new functions have similar but not identical signature; this change means that we don't have to pass things to autobuild() which are not needed for building, but rather just for the meta JSON; and vice versa.
giordano
left a comment
giordano
left a comment
There was a problem hiding this comment.
Looks good to me, only a minor comment about a test
| Product[LibraryProduct("libfoo", :libfoo), FrameworkProduct("fooFramework", :libfooFramework)], | ||
| [Dependency("Zlib_jll")]; | ||
| ) | ||
| println(meta_json_buff, JSON.json(dict)) |
There was a problem hiding this comment.
Isn't there anything interesting we can test on the dictionary?
There was a problem hiding this comment.
I am sure one could. For example, by comparing it with the decoded JSON data in objs down below -- although that'd essentially test the JSON encoder/decoder; not sure if that's so useful?
However, I deliberately tried to restrict the changes in this PR to refactoring, not adding new stuff.
There was a problem hiding this comment.
Sure, but there was a minimal test before 😉
There was a problem hiding this comment.
OK, I must have missed that, what test was there that I accidentally removed? It still serializes and deserializes the dict to JSON and back as before.
3 participants
The two new functions have similar but not identical signature; this change
means that we don't have to pass things to autobuild() which are not needed
for building, but rather just for the meta JSON; and vice versa.
I came up with this while trying to write a patch for issue #856, and trying to understand what data I need to pass where.
The big diff in
testis mostly due to the removal ofmktempdir() do ... endblocks; if one uses the "ignore whitespace" diff mode, one can see the actual (comparatively small) changes.