Replace hand-written bsp4j and bsp4s with generated versions

[agluszak]

Some things are going to break because bsp4j, bsp4s and the typescript definition of the protocol sometimes didn't agree one with another. In other cases the libraries were internally inconsistent because of copy-pasting.

What's going to break:

In bsp4j:

• onConnectWithServer is removed - it wasn't a proper json-rpc notification anyway - probably a leftover from some older version? Also it wasn't defined in the spec at all.
• MessageType.INFORMATION is renamed to MessageType.INFO. The spec clearly says INFO: https://build-server-protocol.github.io/docs/specification#messagetype
• fields in JvmBuildTarget structure were optional in the protocol and bsp4s, so in the corresponding bsp4j class they were moved from constructor parameters to setters. We can either change (the protocol AND bsp4s) or bsp4j. I chose the latter option. But I don't know if these fields being optional make any sense.
• analogous changes in the cpp extension, but it's not used by anyone anyway
• DebugSessionParams had an obligatory dataKind which was illegal according to the spec

In bsp4s:

• CleanCacheResult no longer has a message field. It wasn't defined in the spec.
• all bsp enums now use value for getting the inner value (previously some used id, some used code and some used value)

In spec:

• Made BuildTargetCapabilities' fields optional (as null == false in this case). This isn't a breaking change to the spec, but it will break some code in bsp4j (although it will save some other code from breaking, depending on which constructor it uses). See Replace hand-written bsp4j and bsp4s with generated versions #555 (review)

Non-breaking changes:

• some classes should have had a dataKind, but they didn't. Now they do.
• ${language}Extension.xtend files are broken into separate classes for each structure
• in bsp4s order of definitions is changed - now they are sorted alphabetically
• whitespace changes, I tried to minimize them as much as I could, but again, they were inconsistent in the first place
• hashcode/equals definitions changed (again, they were inconsistent in the first place) and other minor things. For example see https://github.com/build-server-protocol/build-server-protocol/pull/555/files#diff-3a02edaa2405332587baa34149132faeb8012baf0121ff07a1092df100332592 and https://github.com/build-server-protocol/build-server-protocol/pull/555/files#diff-19d83b3030b2fdb7887c55d73380153fd476265395bb70851a1890fbed419b4a
• in JvmEnvironmentItem there was this useless identity assignment

All of the changes above come from the fact that now everything is generated from the smithy model in a consistent way. Hand-written implementations were inconsistent in many places. I didn't introduce any special cases/hardcode anything. But as you can see, all of the breaking changes are very minor/simple.

In bsp4s there's a constant Bsp4s.ProtocolVersion and in bsp4j Bsp4j.PROTOCOL_VERSION with the supported protocol version.

To test this you'll need to do the following

1. Run sbt generate
2. Rest sbt test to show that all the normal tests are still passing, etc

[jastice]

In this PR we have a lot of breaking method name changes, such as buildInitialize -> initializeBuild. From separate discussion, I understand this is for consistency of naming. I also understand we couldn't keep the current naming of everything and have it be both consistent and api/binary compatible. However I'd prefer to keep incompatible changes to a minimum. What do others think?

[agluszak]

I don't think there are that many users of our libraries, so introducing these (minor tbh - that's a few renames and moving setters to constructor parameters or vice versa in a few classes) breaking changes will not cause a leftpad incident ;)

[ckipp01]

I'll try to review this early next week when I'm back, but since this does include some breaking changes I think we should also ping

• @tgodzik
• @lefou

Just so they are aware ASAP and can also provide some input on the breakage.

Also, can you include a few words/steps for others to test if this if they wanted? For example:

To test this you'll need to do the following

1. Run this sbt command, which will generate X
2. Rest this sbt command to show that all the normal tests are still passing, etc

This would be helpful to ensure a quicker review so people don't have to dig through it to find out how to run it.

[tgodzik]

Could we put the renames into a separate commit, so that they are easier to review?

[agluszak]

Could we put the renames into a separate commit, so that they are easier to review?

not really, but I did my best to list all the breaking changes in the PR description

[jastice]

buildInitialize is renamed to initializeBuild and buildShutdown to shutdownBuild. Their params were named "InitializeBuildParams" and "ShutdownBuildParams", so to keep things consistent we either have to rename the method names or the params. All other params are named "${methodName}Params}". BTW, I think we should remove the "build" part of it altogether, because these methods are not related in any way to building. They are related to the lifetime of the server.

They are named based on the json-rpc method name, like build/initialize. Other methods such as buildTarget/resources still retain the name buildTargetResources in bsp4j. So I think we should try to be consistent in the name transformation and maybe rename the parameters instead.

in every extension buildTarget${name}Options method is renamed to ${name}Options (the base protocol doesn't use the buildTarget prefix in method names)

It does, as in above example

In ScalaWorkspaceEdit the list of changes is optional. That's how it was defined in the protocol ¯_(ツ)_/¯ (I agree, it doesn't make any sense, but again, we either break the protocol or the library)

The addition is still new, maybe we can fix it?

in bsp4s order of definitions is changed (from one seemingly random ordering to another seemingly random ordering; I can sort them alphabetically for example)

I guess alphabetical makes sense for minimal conflicts in future additions

constants are renamed from UpperCamelCase to SCREAMING_CASE (e.g. TaskDataKind.CompileReport -> TaskDataKind.COMPILE_REPORT). This one is easy to "un-break" if you insist.

UpperCamelCase is pretty idiomatic for constants in Scala

[agluszak]

It does, as in above example

Yes, of course you are right. I'll fix it then.

I guess alphabetical makes sense for minimal conflicts in future additions

I'll fix it in the next commit.

UpperCamelCase is pretty idiomatic for constants in Scala

This too.

[agluszak]

I reverted the renames and opened #562

[agluszak]

Question to SBT wizards: what should I do to have codegen and xtend tasks run automatically in sequence before each compile task?

[jastice]

Question to SBT wizards: what should I do to have codegen and xtend tasks run automatically in sequence after each compile task?

You could do that, but I don't think you'd want to. compile supports incremental compilation, the code gen afaik doesn't.

[agluszak]

There's also #506

[agluszak]

You could do that, but I don't think you'd want to.

Because the entire codebase for bsp4j and bsp4s are now generated, there's never a situation where you'd want to run a compile without running codegen and xtend beforehand.

[jastice]

Because the entire codebase for bsp4j and bsp4s are now generated, there's never a situation where you'd want to run a compile without running codegen and xtend beforehand.

I suggest creating a dedicated task for generating and compiling then to not mess with expected behavior of tasks too much. Something like

val buildGeneratedCode := {
  codegen.value
  xtend.value
  compile.value
}

My sbt is rusty, might not fully work like this

[jastice]

So I think we can live with some minor breaking changes on generated API level, so from my side it's good to go! I'd like to get feedback from the other tool maintainers though.

[agluszak]

I think all remaining issues have been addressed :) @ckipp01 @jastice @tgodzik @lefou

[agluszak]

Unfortunately our property tests are flaky... This current run failed because a URI generator generated an incorrect URI (not related to my changes in any way). (it was related, the order of constructor arguments was switched around in the previous PR) Some time ago a run failed because the test didn't stop after 6 hours. This happened to me locally a few times as well. Again, it's not related in any way to this PR.

[lefou]

I might have overlooked it, as this PR is quite large. Is there an API now, to get the implemented protocol version? Or did you decide to make version-specific artifacts?

[jastice]

I might have overlooked it, as this PR is quite large. Is there an API now, to get the implemented protocol version? Or did you decide to make version-specific artifacts?

I don't think we've decided anything :)
Adding a version API seems like the easier thing though. Any opinions?

[agluszak]

But there's always been a request for getting the BSP version: in the build/initialize request first the client says the version of BSP it uses and then the server replies with its BSP version

[jastice]

But there's always been a request for getting the BSP version: in the build/initialize request first the client says the version of BSP it uses and then the server replies with its BSP version

The issue here though is knowing which BSP version the library supports, as opposed to the server. It should be somehow clear or discernible from the programmer's perspective. It could be as simple as a hard coded string that can be reall easily navigated to.

[lefou]

But there's always been a request for getting the BSP version: in the build/initialize request first the client says the version of BSP it uses and then the server replies with its BSP version

The issue here though is knowing which BSP version the library supports, as opposed to the server. It should be somehow clear or discernible from the programmer's perspective. It could be as simple as a hard coded string that can be reall easily navigated to.

Exactly. I'd like to use that library hard-coded version to use in the BSP Server implementation to answer the question, which BSP version does this server speak.

[agluszak]

Ahh, I see now. Okay, so let me implement it.

[agluszak]

Implemented @lefou @jastice.

In bsp4s there's a constant Bsp4s.ProtocolVersion and in bsp4j Bsp4j.PROTOCOL_VERSION

[lefou]

Implemented @lefou @jastice.

In bsp4s there's a constant Bsp4s.ProtocolVersion and in bsp4j Bsp4j.PROTOCOL_VERSION

Great, thanks!

[jastice]

If there's no objections by Wednesday, let's merge this, okay?
@adpi2

[adpi2]

Thanks for notifying me. I won't have time to review this PR but I support the main idea and I have no objection to merge it.

[jastice]

6 months in the making! Thank you to everyone involved, especially @Baccata and @agluszak !