Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Feb 4, 2022

Context: 2d5431f
Context: 88d6093
Context: #936
Context: https://github.com/jonpryor/java.interop/commits/jonp-registration-scope

Versioning is hard.

Way back in 3e6a623 we tried to use the GitInfo NuGet package so
that all assemblies would have a version number which contained the
number of commits since GitInfo.txt changed.

This turned out to have unanticipated breakage, and was largely
reverted in 2d5431f for "core" libs like Java.Interop.dll.

This still presents a problem, though: the point to assembly
versioning is to prevent accidental breaking of referencing
assemblies. If we add a new member to Java.Interop.dll but
fail to update the version, then that permits a scenario in which
an app/lib depends on the new member, but is built against a version
missing that member. This results in runtime exceptions.

The whole reason this hasn't been a problem so far is because
Java.Interop.dll has barely changed in years. (Plus, most usage
is hidden behind other layers and libs…)

However, I want this to change: #936 and
jonpryor/java.interop/jonp-registration-scope both add new public
API to Java.Interop.dll, and there are other features and
optimizations we're considering that would also require API changes.
A policy of "no changes" isn't tenable.

Thus: allow Java.Interop.dll built for .NET 6 to have a different
$(Version) than the one built for .NET Standard 2.0.

Fixing this was unexpectedly problematic, as commit 88d6093:

Update[d] Java.Interop.BootstrapTasks.csproj to no longer
<Import/> VersionInfo.targets, as this causes a circular
dependency (VersionInfo.targets uses tasks from
Java.Interop.BootstrapTasks.dll). This is fine, as
Java.Interop.BootstrapTasks.dll doesn't need to be versioned.

Java.Interop.BootstrapTasks.dll doesn't need to be versioned, but
other libraries do, and this change change meant that
VersionInfo.targets was never used, which in turn meant that e.g.
bin/BuildDebug/Version.props was never created.

Re-introduce VersionInfo.targets by "moving" it into
build-tools/VersionInfo/VersionInfo.csproj, and adding
VersionInfo.csproj to Java.Interop.BootstrapTasks.sln.
This allows the Prepare target to create
bin/Build$(Configuration)/Version.props.

This in turn requires moving Java.Interop.BootstrapTasks.sln into
topdir, so that it can use global.json and the
Microsoft.Build.NoTargets version entry.

Next, update Java.Interop.csproj so that $(Version) is
conditional on $(TargetFramework). This allows it to continue
using version 0.1.0.0 for .NET Standard 2.0, and begin using
6.0.200.* for .NET 6+ builds.

Update the default $(Version) value to be 6.0.200.* instead of
0.1.*.0, matching the .NET SDK for Android workload version.

Finally -- and most of the "noise" in this commit -- "cleanup and
unify" the disparate build systems. The above $(Version) changes
necessitate updating the make prepare rule & msbuild /t:Prepare
target.

Clean this up: the dotnet-based MSBuild environment is now our
Source Of Truth, and the make targets are updated to rely on the
Prepare target instead of duplicating most of the logic.

Update msbuild.mk so that dotnet build is used instead of
msbuild.

While cleaning up Makefile, remove other cruft such as the explicit
gcc calls to build the NativeTiming lib (we have a .csproj as
of 5c756b1; use it!), and rationalize
Java.Runtime.Environment.dll.config generation.

Rename the <GenerateVersionFile/> task to <ReplaceFileContents/>,
as that better describes what this task actually does.

Rename the JdkInfo.JdksRoot property to JdkInfo.JdkRoot, as this
better conveys actual semantics, and -- as part of "makefile cleanup"
-- stop probing for /System/Library/JavaVirtualMachines on macOS,
which removes a warning from make prepare:

warning : Could not get information about JdksRoot path `/Library/Java/JavaVirtualMachines`:
Could not find required file `jar` within `/Library/Java/JavaVirtualMachines`; is this a valid JDK?

(No, not is is not a valid JDK. It never was, and I now have to
believe that this has always warned.)

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Feb 4, 2022
Context: 2d5431f
Context: 88d6093
Context: dotnet#936
Context: https://github.com/jonpryor/java.interop/commits/jonp-registration-scope

Versioning is hard.

Way back in 3e6a623 we tried to use the `GitInfo` NuGet package so
that *all* assemblies would have a version number which contained the
number of commits since `GitInfo.txt` changed.

This turned out to have unanticipated breakage, and was largely
reverted in 2d5431f for "core" libs like `Java.Interop.dll`.

This still presents a problem, though: the *point* to assembly
versioning is to prevent accidental breaking of referencing
assemblies.  If we add a new member to `Java.Interop.dll` but
*fail to update the version*, then that *permits* a scenario in which
an app/lib depends on the new member, but is built against a version
missing that member.  This result in runtime exceptions.

The whole reason this hasn't been a problem so far is because
`Java.Interop.dll` has barely changed in *years*.  (Plus, most usage
is hidden behind other layers and libs…)

However, *I want this to change*: dotnet#936 and
jonpryor/java.interop/jonp-registration-scope both *add* new public
API to `Java.Interop.dll`, and there are other features and
optimizations we're considering that would also require API changes.
A policy of "no changes" isn't tenable.

Thus, the first priority: allow `Java.Interop.dll` built for .NET 6 to
have a different `$(Version)` than the one built for .NET Standard 2.0.

Fixing this was unexpectedly problematic, as commit 88d6093:

>  Update[d] `Java.Interop.BootstrapTasks.csproj` to *no longer* `<Import/>`
> `VersionInfo.targets`, as this causes a circular dependency
> (`VersionInfo.targets` uses tasks from
> `Java.Interop.BootstrapTasks.dll`).  This is fine, as
> `Java.Interop.BootstrapTasks.dll` doesn't need to be versioned.

`Java.Interop.BootstrapTasks.dll` doesn't need to be versioned, but
*other libraries **do***, and this change change meant that
`VersionInfo.targets` was *never* used, which in turn meant that e.g.
`bin/BuildDebug/Version.props` was never created.

Re-introduce `VersionInfo.targets` by "moving" it into
`build-tools/VersionInfo/VersionInfo.csproj`, and adding
`VersionInfo.csproj` to `Java.Interop.BootstrapTasks.sln`.
This allows the `Prepare` target to create
`bin/Build$(Configuration)/Version.props`.

Next, update `Java.Interop.csproj` so that `$(Version)` is
*conditional* on `$(TargetFramework)`.  This allows it to continue
using version 0.1.0.0 for .NET Standard 2.0, and begin using
6.0.0.* for .NET 6+ builds.

Finally -- and most of the "noise" in this commit -- "cleanup and
unify" the disparate build systems.  `make prepare all` is one world,
while `dotnet` is a different and overlapping world.

Clean this up: the `dotnet`-based MSBuild environment is now our
Source Of Truth, and the `make` targets are updated to rely on the
`Prepare` target instead of duplicating most of the logic.

While cleaning up `Makefile`, remove other cruft such as the explicit
`gcc` calls to build the `NativeTiming` lib (we have a `.csproj` as
of 5c756b1; use it!), and rationalize
`Java.Runtime.Environment.dll.config` generation.
@jonpryor jonpryor force-pushed the jonp-net6-versioning branch from 49663cf to 2f1234c Compare February 4, 2022 02:35
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Feb 4, 2022
@jonpryor jonpryor force-pushed the jonp-net6-versioning branch from e4dfe26 to 4f436bc Compare February 4, 2022 17:18
The **Windows - .NET Core** > **Prepare Solution** step is
failing:

    ##[error]build-tools\VersionInfo\VersionInfo.csproj(0,0): Error MSB4236: The SDK 'Microsoft.Build.NoTargets' specified could not be found.
    D:\a\1\s\build-tools\VersionInfo\VersionInfo.csproj : error MSB4236: The SDK 'Microsoft.Build.NoTargets' specified could not be found.

…which doesn't make sense, as `Microsoft.Build.NoTargets` is
versioned in `global.json`, and similar usage works in
xamarin-android…

Hypothesis: it works elsewhere because the `.sln` is in the same
dir as the `global.json`, which isn't the case here.

Theory: adding `build-tools/global.json` will fix things.

Time to test!

Result: it worked.

Thus, final fix: move `Java.Interop.BootstrapTasks.sln` into topdir,
allowing us to remove `build-tools/global.json`.

Other cleanups done as well.
</Project>

<Target Name="_SetInformationalVersion"
BeforeTargets="GetAssemblyVersion;GetPackageVersion">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right: https://github.com/dotnet/sdk/blob/2a515cdbd8f6be1b019ae2c8d7f21952592f0697/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets#L205-L214

We might want to manually check the Java.Interop.dll in the next java.interop bump? Will we remember?

    RunTests:
       packages/nunit.consolerunner/3.12.0/tools/nunit3-console.exe  bin\TestRelease\generator-Tests.dll  --result="TestResult-generator-Tests.xml" --output="bin\TestRelease\TestOutput-generator-Tests.txt"
      'packages' is not recognized as an internal or external command,
      operable program or batch file.
    ##[error]build-tools\scripts\RunNUnitTests.targets(27,5): Error MSB3073: The command " packages/nunit.consolerunner/3.12.0/tools/nunit3-console.exe  bin\TestRelease\generator-Tests.dll  --result="TestResult-generator-Tests.xml" --output="bin\TestRelease\TestOutput-generator-Tests.txt"" exited with code 9009.
    D:\a\1\s\build-tools\scripts\RunNUnitTests.targets(27,5): error MSB3073: The command " packages/nunit.consolerunner/3.12.0/tools/nunit3-console.exe  bin\TestRelease\generator-Tests.dll  --result="TestResult-generator-Tests.xml" --output="bin\TestRelease\TestOutput-generator-Tests.txt"" exited with code 9009.

The assumption is that we're not doing `nuget restore` now, so
the package is installed into `~/.nuget/…`, and thus isn't found.

Fix that by "using" `%(PackageReference.GeneratePathProperty)`
in the bootstrap tasks so that we can obtain
`$(PkgNUnit_ConsoleRunner)`, and *generate* that during
`dotnet build -t:Prepare`.  That way `RunNUnitTests.targets`
has access to the value.
Pobst doesn't want us to use `6.0.200`; use `6.0` for GitInfo.txt.

Remove the `globalPackagesFolder` override; it shouldn't be needed
anymore, given dab4b89.
@jpobst
Copy link
Contributor

jpobst commented Feb 7, 2022

This allows it to continue using version 0.1.0.0 for .NET Standard 2.0, and begin using 6.0.200.* for .NET 6+ builds.

I do not believe we can allow the Java.Interop.dll version to "float" like this.

Imagine I install .NET 6.0.300.0, build my binding library with it, and publish the NuGet. All binding libraries take an assembly reference on Java.Interop.dll, so my library will require Java.Interop.dll 6.0.300.0.

However, the only versioning that NuGet is aware of is net6.0-android31.0, meaning any project targeting that can add a package reference to my NuGet and expect it to work.

If someone is still using .NET 6.0.200.0, NuGet will allow them to reference the binding package, but their compilation will always fail with the same error we had to back out in 2d5431f.

error CS1705: Assembly 'Microsoft.Maui' with identity 'Microsoft.Maui, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null'
uses 'Java.Interop, Version=6.0.300.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'
which has a higher version than referenced assembly
'Java.Interop' with identity 'Java.Interop, Version=6.0.200.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'

We can hardcode Java.Interop.dll to <Version>6.0.0.0</Version> if we want to, because that is the same "contract" as "net6.0". But we cannot change the version (or api) in minor or patch versions of NET 6. The next available version would be 7.0.0.0.

@jonpryor jonpryor marked this pull request as draft February 8, 2022 17:34
@jonpryor
Copy link
Contributor Author

Superseded by PR#952.

@jonpryor jonpryor closed this Feb 10, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants