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

Use a custom MSBuild task #112

Closed
adamralph opened this issue Nov 20, 2018 · 21 comments
Closed

Use a custom MSBuild task #112

adamralph opened this issue Nov 20, 2018 · 21 comments
Labels
breaking This change could break current consumers enhancement New feature or request wontfix This will not be worked on

Comments

@adamralph
Copy link
Owner

adamralph commented Nov 20, 2018

This will replace the use of the Exec task to run MinVer.dll as an executable, with the use of a custom task within that assembly. This is dependent on either #256, or changes in LibGit2Sharp which @bording is aware of.

One functional effect of this change is that the MSBuild verbosity level will be respected, which means that MinVerVerbosity will be ignored (probably deprecated initially, with a warning message). This could be considered a breaking change. It will also mean that the calculated version will not be outputted to the console (when the MSBuild verbosity level is detailed) as a side effect of using the Exec task.

@adamralph adamralph added breaking This change could break current consumers on-hold This can't be done yet enhancement New feature or request labels Nov 22, 2018
@adamralph
Copy link
Owner Author

I've been thinking about the pros and cons of introducing this in 1.0.0, but still delegating to an executable for now. In order to do proper logging, the executable can write to stderr with prefixes that indicate the log level, so that the task can parse them and propagate them back to the MSBuild logger.

Pros

  • removes stdout noise
  • removes the need for MinVerVerbosity (the logging level will be inherited from MSBuild)
  • nicer errors than with the Exec task
  • simplifies targets file

Cons

  • involves managing System.Process
  • requires listening to stderr and parsing (I'm not sure how reliable this is)
  • requires another project sitting between MinVer and MinVer.Lib, and more complex packaging

@bording
Copy link
Collaborator

bording commented Nov 23, 2018

involves managing System.Process
requires listening to stderr and parsing (I'm not sure how reliable this is)

There is a ToolTask type that would be worth looking at to see if it helps with these.

requires another project sitting between MinVer and MinVer.Lib, and more complex packaging

It seems like it could be possible to use the minver-cli output instead of needing a separate project.

@adamralph
Copy link
Owner Author

One thing to bear in mind is that there is no such thing as a "current log level" in the context of an MSBuild task, since the host process can have many different loggers attached to it, each configured to a different level. That means that every message, including all debug and trace messages, will have to be output to stderr, and parsed, on every run.

Also, I'm not entirely sure that we can just use minver-cli for this, since we'd also need a way of communicating the MINVERXXXX error and warning codes back to the task, which would either mean adding them to the message (either always or via a "hidden" option) or having some kind of convention based on the pattern of the text. Or, it could always be the same error code, but that seems clunky.

Putting aside the stdout noise (one line) and the slightly uglier error messages, I believe the only significant functional difference here is the removal of MinVerVerbosity, and allowing each of the configured loggers to choose which level of messages it receives. I'm leaning toward that not being enough of a benefit to outweigh the costs.

@bording
Copy link
Collaborator

bording commented Nov 24, 2018

The fact that there is no way to know the configured log level is rather annoying.

I agree with your assessment that it doesn't make sense to go ahead and try for a task now just to get rid of MinVerVerbosity.

I'd say this can wait until it's feasible to actually use LibGit2Sharp directly from an MSBuild task.

@adamralph
Copy link
Owner Author

Another thing to consider is that currently, MinVer is completely platform agnostic. I.e. it will work for any target framework supported by SDK-style projects. If we introduce a task assembly, then target frameworks start to become something to worry about as detailed in https://natemcmaster.com/blog/2017/07/05/msbuild-task-in-nuget/

@bording
Copy link
Collaborator

bording commented Nov 28, 2018

To cover everything currently supported, net461;netstandard2.0 should be enough.

@adamralph
Copy link
Owner Author

I tested MinVer in a net20 project and it worked. 😉

@bording
Copy link
Collaborator

bording commented Nov 30, 2018

@adamralph While that's cool, I'm not sure how that is relevant?

@adamralph
Copy link
Owner Author

@bording it shows that we will lose platform support by doing this. Of course, net20 isn't relevant, but perhaps some others may be.

@bording
Copy link
Collaborator

bording commented Nov 30, 2018

The target platforms of the projects using MinVer aren't relevant for this. What matters is the version of .NET that VS and the SDK are using.

Since we only support SDK-style projects, that means VS 2017, so for VS and its MSBuild, we need net461. (Or possibly net46. I'd need to doublecheck to be 100% on that). For building from the .NET Core SDK, we need something that works with .NET Core 2.x, which is where the netstandard2.0 assembly comes in.

Building two task assemblies, net461;netstandard2.0, means we've covered all the bases, and nothing is lost.

@adamralph
Copy link
Owner Author

@ursenzler is it correct that you discovered that the target platform of the project does matter?

@ursenzler
Copy link

@adamralph yes. You cannot use a build task with target netstandard2.0 (the target the MinVerTask would have due to LibGit2) in a project targeting netstandard1.0 for example.
https://twitter.com/terrajobst/status/1064589484740603906

@bording
Copy link
Collaborator

bording commented Dec 3, 2018

@ursenzler If you're using the GitVersion package has an example, then I think you might be misunderstanding what's going on because of some choices made by the GitVersion maintainers. The latest version of that package actually has dependencies, and that's going to be what't blocking the package installation on a netstandard1.0 project. It has nothing to do with what the underlying build task is built against.

If done properly, a versioning package like this should have no dependencies, so it will work in any project regardless of the target framework.

@ursenzler
Copy link

@bording true, but MinVer has a dependency to LibGit2, doesn't it?

@bording
Copy link
Collaborator

bording commented Dec 4, 2018

It does, but that's not relevant here because the package will never have a LibGit2Sharp dependency. It will always be bundled inside the package, and will never creep into your projects as a dependency.

@adamralph
Copy link
Owner Author

This would probably allow #244.

@adamralph adamralph modified the milestone: 2.0.0 Sep 10, 2019
@adamralph adamralph added on-hold This can't be done yet and removed on-hold This can't be done yet labels Sep 14, 2019
@adamralph
Copy link
Owner Author

No longer blocked by LibGit2Sharp, but unsure if I want to do it anyway.

@adamralph
Copy link
Owner Author

@bording I'm leaning towards closing this as a wontfix.

The only benefit I can think of is the removal of MinVerVerbosity and I don't think many people (myself included) care much about it.

There are several costs I can think of:

  • The package will have to be multi-targetted for net461 and netstandard2.0.
  • Depending on how MSBuild develops in the future, those TFM's may need to change again, which will lead to more releases and versioning considerations.
  • The code will increase in complexity, since the MinVer package will have to switch to using the MSBuild task API, while the minver-cli package will remain as a CLI.

@adamralph adamralph removed the on-hold This can't be done yet label Nov 17, 2019
@bording
Copy link
Collaborator

bording commented Nov 17, 2019

Now that you're no longer using LibGit2Sharp, I agree that there's less of a reason to do this. Even with a custom task, you're still calling a separate process and having to parse output.

@adamralph adamralph added the wontfix This will not be worked on label Nov 17, 2019
@adamralph adamralph closed this as not planned Won't fix, can't repro, duplicate, stale Jul 9, 2022
@adamralph
Copy link
Owner Author

Re-opening based on #940 /cc @bording

@adamralph adamralph reopened this Jan 17, 2024
@adamralph adamralph removed the wontfix This will not be worked on label Jan 17, 2024
@adamralph adamralph added the wontfix This will not be worked on label Aug 15, 2024
@adamralph
Copy link
Owner Author

withdrawn in favour of #1021

@adamralph adamralph closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change could break current consumers enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants