Use the Go Jsonnet port instead of the C++ port #113
Conversation
|
It looks like the transitive dependencies for google/go-jsonnet (in particular, on rules_go) aren't picked up properly here. I'll look into that. |
|
In my old job we already switched over to the go version because we hat compiling issues on some non LTS Ubuntu versions. While I am generally in favour of this change I also think we should be a bit more careful with it, e.g. now people need to pull in the go toolchain which they might have not have needed before. Also now the c++ version of the formatter is gone and we should think if we should pull that in separately. So even though we are not on 1.0 yet I would still consider it a breaking change and maybe even posting something on bazel-dev or bazel-discuss. WDYT? |
|
I figured out what was keeping the tests from passing here. I opened google/go-jsonnet#292, which I'll show use of from here as well shortly. |
|
@Globegitter, where does this repository use jsonnetfmt? As you said, I do consider this to be a breaking change, at least to consuming projects' WORKSPACE files. The use of the The release numbers have been low here, seeming far away from 1.0. It's tempting to say that we could keep the existing WORKSPACE "interface" to the C++ Jsonnet port the same, and introduce new macros to use the Go port instead, with the threat of deprecating the former. I don't know whether I'll have enough time in the coming week or two to work through that complexity, though. |
| @@ -25,13 +25,22 @@ external repositories for Jsonnet: | |||
| ```python | |||
| http_archive( | |||
| name = "io_bazel_rules_jsonnet", | |||
| # TODO: Update this to reflect a later release. | |||
There was a problem hiding this comment.
NB: I'm publishing this here in advance. Some subsequent coordination is necessary.
|
Yeah I know that this repository has not been depending on the formatter, but at my previous company we where depending on it and other people might have made use of it as well, so it might still be nice to provide the c++ formatter for those cases. Yeah I like the idea of keeping both at the same time for a migration window. I am happy to deprecate the c++ version immediately though. I won't have much time the next couple of weeks, but might also be able to spend some time on this after if you don't get around to it. |
By default, Bazel will use the Go port of Jsonnet. To use the C++ port of Jsonnet instead, invoke Bazel with the "--define jsonnet_port=cpp" command-line flag. To select the Go port explicitly, invoke Bazel with the "--define jsonnet_port=go" command-line flag.
|
@Globegitter, please see my most recent commit a0444b9. It makes both ports of Jsonnet available, with selection via command-line flag. I didn't see an easier way to use Bazel's configurable build attributes system without relying on a command-line flag. The default choice is the Go port. If you feel strongly that we should retain the C++ port as the default choice, I can do that. My proposal here—including the deprecation warning in the README.md file—indicates my preference for switching to the Go port. |
|
Note that we could add more CI targets to the .bazelci/presubmit.yml file to exercise this repository with the possible |
|
I've been using this to good effect in a couple of projects of mine. Has anyone else tried it? Is there anything to adjust before merging it? |
|
@seh Sorry this dropped of my radar I will take a look again in the next couple of days. |
|
@seh This looks really good to me can we just swap the default back to cpp for now, so we can release it fully backwards-compatible and then we can change it in a subsequent commit and make another release. So people have one release to test things out and report any potential issues. |
Yes, I think it will be a small code change to do so. It's slightly more work to adjust the documentation, but I'll take care of it. I will share a separate patch (along this same branch) for the change in default, so that we retain the current head as a destination for the follow-on release when we really commit to the Go port being the default. |
Retain use of C++ as the default port for one more release, with the intention to change the default to the Go port and eventually remove support for the C++ port.
|
Please see commit 0670955 for the requested change in the default Jsonnet port. |
The selected commit is the latest along the "master" branch as of 24 August 2019.
|
@Globegitter, do you have any other suggestions here? We've had to switch over to using my own fork in our internal projects, but I'd prefer to rely on this repository instead. |
|
@seh Sorry for the delay,and thanks for the great contribution. |
|
@seh sorry again for the delay, I also released version 0.2.0 now. If you want you can already submit a PR to flip the default back again and if there are now issues reported I can release it in one or two weeks time. |
|
Thank you! I expect that I can post that follow-on patch to switch the default within a couple of days. |
As threatened in bazel-contrib#113 in mid-2019, change the default Jsonnet tool to the Go port, leaving the C++ port available for selection via the "--define" command-line flag to set the "jsonnet_port" build variable accordingly.
|
See #129 for following through on changing the default Jsonnet port. |
As threatened in bazel-contrib#113 in mid-2019, change the default Jsonnet tool to the Go port, leaving the C++ port available for selection via the "--define" command-line flag to set the "jsonnet_port" build variable accordingly.
As threatened in #113 in mid-2019, change the default Jsonnet tool to the Go port, leaving the C++ port available for selection via the "--define" command-line flag to set the "jsonnet_port" build variable accordingly.
The Jsonnet project's development focus has shifted to the Go language port, with all the recent performance optimizations landing there. Change rules_jsonnet to use the Go port instead of the C++ port unconditionally.
While it would be possible to expose this choice to users of this package, perhaps by way of introducing a sibling function to
jsonnet_repositories, or introducing a parameter to that function, I don't think it's worth the maintenance burden to do so.@sparkprime and @sbarzowski, your guidance would be welcome here.