Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

output version number parts as properties and set AssemblyVersion #113

Merged
merged 1 commit into from
Nov 22, 2018

Conversation

adamralph
Copy link
Owner

@adamralph adamralph commented Nov 20, 2018

@bording this is a horrid a slightly horrid attempt at solving #109.

Functionally, it's fine, although the downside is a whole lot more noise in the build output. The stdout side-effect of using the Exec task goes from one line to four (although this will go away with #112):

Version:2.0.0-alpha.0.1+build.42
Major:2
Minor:0
Patch:0

Code-wise, it's awful. Specially since taking a substring directly on @(MinVerOutputVersion), etc. didn't work (or at least my MSBuild-fu failed me) so I had to do that in a second series of property assignments.

Can you think of a better way? 🤞 Resolved in #115. 🤘

One more thing is that this may also require a re-consideration of the MinVerVersionOverride scenario, since currently, the only properties which would be set are Version and PackageVersion. Either the README would need to be updated to reflect the added burden on the user to set MinVerMajor, MinVerMinor, MinVerPatch, and AssemblyVersion manually, which is at least a bit horrible, or the version override should be fed into MinVer.dll so that it can be pulled apart into those properties. This is marked as WIP on that basis.

Closes #109.
Closes #116.

@adamralph adamralph added this to the 1.0.0 milestone Nov 20, 2018
@adamralph
Copy link
Owner Author

Another idea is to stick with the current behaviour by default, and have a MinVerExtendedProperties option which can be set to true when these extra properties are required. The option would deprecated and ignored as part of #112.

@bording
Copy link
Collaborator

bording commented Nov 20, 2018

See my comment on #109 on some thoughts about the need for these changes. Can we discuss those before I dig into the details of what this PR is doing and see if there's a better way to do it?

MinVer.Lib/Version.cs Outdated Show resolved Hide resolved
MinVer.Lib/Version.cs Outdated Show resolved Hide resolved
MinVer/build/MinVer.targets Outdated Show resolved Hide resolved
@adamralph
Copy link
Owner Author

@bording I've amended the commit to reflect our recent discussions. I think we can leave out the pre-release identifiers until a use-case arises.

@adamralph adamralph changed the title [WIP] output version number parts as properties [WIP] output version number parts as properties and set AssemblyVersion Nov 21, 2018
@adamralph
Copy link
Owner Author

@bording I think the only remaining considerations here are:

...the downside is a whole lot more noise in the build output...

The only way to get around that is to wait for #112 before implementing these features. I think it's better to live with the noise for now.

And:

One more thing is that this may also require a re-consideration of the MinVerVersionOverride scenario, since currently, the only properties which would be set are Version and PackageVersion. Either the README would need to be updated to reflect the added burden on the user to set MinVerMajor, MinVerMinor, MinVerPatch, and AssemblyVersion manually, which is at least a bit horrible, or the version override should be fed into MinVer.dll so that it can be pulled apart into those properties. This is marked as WIP on that basis.

I'm really not sure what the best course of action is. It's good that the override completely bypasses invocation of MinVer.dll, since then any bugs that arise can be temporarily routed around without having to make commits to branches (to temporarily remove MinVer). That could be a significant advantage when dealing with old support branches, etc. But if we want to preserve that, then the override becomes much more laborious. So it's a trade off between effectiveness of the override, and effort involved in specifying it.

But then this got me thinking about whether we really need an override. Or if we do, should it just be MINVERDISABLE, which completely switches it off. In that case, you'd have to temporarily specify VERSION, PACKAGEVERSION, and any other properties on your build server. But getting back to whether we really need an override, I'm not sure we do. After all, the version only really matters when the current commit is tagged. So to "fix" your versioning, all you need to do is push a tag. And since the logic in MinVer is so (relatively) simple, I don't think anyone will get into the "I don't know what's going on" situation described in the FAQ. But even if we keep that FAQ, we could probably change the answer to just "create a tag".

MinVer/build/MinVer.targets Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
targets/Targets.csproj Outdated Show resolved Hide resolved
@bording
Copy link
Collaborator

bording commented Nov 22, 2018

But then this got me thinking about whether we really need an override. Or if we do, should it just be MINVERDISABLE, which completely switches it off. In that case, you'd have to temporarily specify VERSION, PACKAGEVERSION, and any other properties on your build server. But getting back to whether we really need an override, I'm not sure we do. After all, the version only really matters when the current commit is tagged. So to "fix" your versioning, all you need to do is push a tag. And since the logic in MinVer is so (relatively) simple, I don't think anyone will get into the "I don't know what's going on" situation described in the FAQ. But even if we keep that FAQ, we could probably change the answer to just "create a tag".

What are the scenarios where you're really be wanting to override the calculated version? If it's because MinVer is "broken", would you really need to just alter some build server settings to fix it? Why not commit some changes, like removing the package and manually versioning things in that case?

And if you just need it to be some different version for some reason, it seems like tagging the repo is simpler than going into your build server settings to add environment variables.

So, based on that, I agree that we could just remove the override and assume that if the package is installed, it should be trying to version things.

@adamralph adamralph force-pushed the moar-properties branch 2 times, most recently from d94e3f4 to 0494778 Compare November 22, 2018 08:10
@adamralph adamralph changed the title [WIP] output version number parts as properties and set AssemblyVersion output version number parts as properties and set AssemblyVersion Nov 22, 2018
@adamralph adamralph merged commit e6761e7 into master Nov 22, 2018
@adamralph adamralph deleted the moar-properties branch November 22, 2018 08:24
@adamralph
Copy link
Owner Author

Released in alpha 16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants