NuGetUtils update #1645

asbjornu · 2019-03-21T19:40:34Z

Rebase of #1634. Resolves #1634.

asbjornu · 2019-03-21T19:48:38Z

This is puzzling. This PR is just a rebase of #1634 (so all the merge commits are removed), but the build fails on this while it's successful in #1634. Any ideas why, @arturcic?

arturcic · 2019-03-22T07:25:06Z

Looks like everywhere the same test is failing OutputsShouldMatchVariableProvider

asbjornu · 2019-03-22T12:27:02Z

Any ideas why that fails here and not in #1634, @arturcic? I don't get it.

arturcic · 2019-03-22T12:28:05Z

Not really, we need to check if there is any difference between these 2 branches

asbjornu · 2019-03-22T12:55:06Z

@arturcic, I did a fresh git reset and git rebase now and that seems to have fixed the problem. Things are at least green. 🤷‍♂️

arturcic · 2019-03-22T12:56:22Z

Great! My question is why do we have 2 PRs?

asbjornu · 2019-03-22T12:57:52Z

Just to get rid of these: c2f3610, 9d8eb38, b608f96, 854f16b.

arturcic · 2019-03-22T12:59:55Z

So I guess this PR will get merged instead of the other one

asbjornu · 2019-03-22T13:05:12Z

Yes, this resolves #1634 so as soon this is merged, it will be closed automatically. It would be neat if GitHub supported rebase instead of merge for the "Update branch" button, but alas it doesn't. I've just installed Autorebase which might help us with that. I'll try that on #1634 shortly.

asbjornu · 2019-03-23T23:26:23Z

Sigh. Seems like my rebase failed for some reason.

src/GitVersionTask/GitVersionTask.csproj

asbjornu · 2019-03-24T19:02:59Z

Thanks for figuring that out, @dazinator. I think I got the rebase right now, but notice that 3b28212 (and my rebased de31124) reintroduces the Logger.Reset() calls that @tpaxatb removed in #1644. I suppose that's a mistake, @stazz?

tpaxatb · 2019-03-25T13:55:50Z

Thanks for figuring that out, @dazinator. I think I got the rebase right now, but notice that 3b28212 (and my rebased de31124) reintroduces the Logger.Reset() calls that @tpaxatb removed in #1644. I suppose that's a mistake, @stazz?

I'm going to assume that it's actually ok as is, since the logger.reset is actually paired with a setloggers call at the beginning of the function (rather than in a constructor as it was previously...)

asbjornu · 2019-03-25T14:20:52Z

Thanks for the insight, @tpaxatb. Then I guess this just needs review and approval from @dazinator and we should be good to go. Do you agree, @arturcic?

arturcic · 2019-03-25T14:22:05Z

Agree

stazz · 2019-03-25T19:35:51Z

@asbjornu Yes, that is indeed a mistake. Rebase ended up being a bit messy even though I followed instructions, sorry about that. I have close to zero experience with rebasing stuff, as I've never worked with GIT repo which follows SVN workflow.

But yeah if @tpaxatb was ok with that, then it's fine. At least nothing should really interact directly with the MSBuild things anymore. The question is, does the NuGetUtils.MSBuild.Exec task factory handle logging using IBuildEngine in some special way? I think no, but maybe @tpaxatb can shed some insight on this? Currently the task factory is using the IBuildEngine provided in Initialize method when it performs pre-checks, and the task proxy will just redirect all output of invoked task to the IBuildEngine that it receives.

@dazinator Do I need to do any changes anywhere? The change request about the .csproj file seems to be not-relevant anymore, at least when looking at current state of https://github.com/asbjornu/GitVersion/blob/feature/NuGetUtilsUpdate/src/GitVersionTask/GitVersionTask.csproj .

stazz · 2019-03-27T18:39:50Z

Hmm, actually, it seems I have some bug introduced in recent commits, despite the fact that I've added tests for the NuGetUtils.MSBuild.Exec. I'll investigate and back to you.

stazz · 2019-03-27T21:52:01Z

Hmm, actually, it seems I have some bug introduced in recent commits, despite the fact that I've added tests for the NuGetUtils.MSBuild.Exec. I'll investigate and back to you.

Found the bug, it was only appearing in some quite special circumstances (e.g. having multiple assembly files in same package or having certain dependencies). I've fixed it now for 2.0.5, and updated my fork.

feitzi · 2019-05-06T14:04:56Z

any updates?

stazz · 2019-05-08T09:49:24Z

Hey @feitzi , for my part, everything should be ok, I think. The NuGetUtils task factory has been tested and put into use by GitVersionTask. I am not sure what the conflicts mentioned here are exactly about - it seems that GitVersionTask is using workflow from SVN originally (based on some message I saw in this or another issue).
@dazinator @asbjornu if I need to still do something, feel free to instruct what needs to be done. :)

…Utils.MSBuild.Exec.

…ory name in .props file.

…successfully now. Also modified the task factory parameter as needed.

…g with ITaskItems (seen as String arrays to GitVersionTask).

…e thorough check later though.

…down the amount of process invocations used by the task factory.

…t fixes.

asbjornu

I've rebased and resolved conflicts, but I was unable to figure out how to properly apply the changes made by @arturcic in 3e37390 to your complete rewrite of GenerateGitVersionInformation, UpdateAssemblyInfo and WriteVersionInfoToBuildLog @stazz.

I now also noticed the change of GenerateGitVersionInformation and GitVersionTaskBase to static, which feels very strange for the latter class especially. Having a Base class that can't be inherited from doesn't make much sense, imho. Overall I believe what the PR does is good, I'm just unsure of the details in how it's implemented and now with the changes introduced in just about all classes this PR touches, I'm unsure of the validity of this PR as a whole.

Would it be too much to ask that you made a fresh attempt at implementing NuGetUtils without the major refactoring to static and such you've done in this PR, @stazz? That would make it much easier to review and merge. In its current state, I'm not comfortable merging this at least.

asbjornu · 2019-05-08T10:27:13Z

src/GitVersionTask/GenerateGitVersionInformation.cs


-    public class GenerateGitVersionInformation : GitVersionTaskBase
+    public static class GenerateGitVersionInformation


@stazz, why the change to static class here?

The reason is that the class has no state on its own anymore (instead, the Input and Output inner classes are used), so no instances of it will be created either.

I see. Thanks for the explanation.

asbjornu · 2019-05-08T10:29:05Z

src/GitVersionTask/GenerateGitVersionInformation.cs


-            GitVersionInformationFilePath = Path.Combine(workingDirectory, fileName);
+        public sealed class Input


@stazz, why isn't any form of inheritance used across the different Input classes (and Output classes, for that matter)?

I simply forgot to do that. I'll fix this on Sunday.

asbjornu · 2019-05-08T10:29:51Z

src/GitVersionTask/GenerateGitVersionInformation.cs

+            public bool NoFetch { get; set; }
+        }
+
+        private static Boolean ValidateInput( this Input input )


@stazz, why isn't this method placed inside the Input class itself?

No particular reason. I guess the validity input depends a bit on a context (which I admit in this case is exactly one). Can move it as instance method of Input class on Sunday.

asbjornu · 2019-05-08T10:30:44Z

src/GitVersionTask/GetVersion.cs


-    public class GetVersion : GitVersionTaskBase
+    public static class GetVersion


@stazz, why the change to static here?

Just like GenerateGitVersionInformation (and one other task too), this class has no state on its own anymore, so no instances of it will be created either.

stazz · 2019-05-08T11:08:44Z

The static is because there is no longer need to extend ITask. Instead, the entrypoint acts just like a normal C# entrypoint, with an addition of having complex typed parameters (and not just String[]). That pretty much eliminates the need to use class hierarchy for the tasks contained in the assembly as well.

So did I undestand you correctly @asbjornu that are you suggesting that I make a fresh fork out of the latest tip of GitVersion repository, and re-apply changes for NuGetUtils? That makes sense for me, as I have a feeling that the change log is now way too convoluted. The work load of doing so is not too big, IMO, as the changes were relatively straightforward, but I think I'll have time for that only on this Sunday.

With the full re-apply of changes, I also hope it will be easier to see the reasoning behind the resulting static entrypoints and no class hierarchy for tasks.

asbjornu · 2019-05-09T20:52:03Z

@stazz:

So did I undestand you correctly @asbjornu that are you suggesting that I make a fresh fork out of the latest tip of GitVersion repository, and re-apply changes for NuGetUtils?

Yes, exactly.

That makes sense for me, as I have a feeling that the change log is now way too convoluted. The work load of doing so is not too big, IMO, as the changes were relatively straightforward, but I think I'll have time for that only on this Sunday.

I'm very happy to hear that! Awesome!

With the full re-apply of changes, I also hope it will be easier to see the reasoning behind the resulting static entrypoints and no class hierarchy for tasks.

👍

stazz · 2019-05-12T15:10:06Z

@asbjornu This is now opened as #1677 . :)

asbjornu · 2019-05-20T07:11:47Z

Resolved by #1677.

asbjornu force-pushed the feature/NuGetUtilsUpdate branch from 3ef9634 to 3f525c4 Compare March 22, 2019 12:35

tpaxatb mentioned this pull request Mar 22, 2019

Add WiX define constants #1647

Merged

asbjornu mentioned this pull request Mar 23, 2019

Updating UtilPack.NuGet.MSBuild to NuGetUtils.MSBuild.Exec #1634

Closed

asbjornu force-pushed the feature/NuGetUtilsUpdate branch from 3f525c4 to 37b2aa1 Compare March 23, 2019 23:19

dazinator requested changes Mar 24, 2019

View reviewed changes

src/GitVersionTask/GitVersionTask.csproj Outdated Show resolved Hide resolved

asbjornu force-pushed the feature/NuGetUtilsUpdate branch from 37b2aa1 to 99300d1 Compare March 24, 2019 18:55

asbjornu force-pushed the feature/NuGetUtilsUpdate branch from 99300d1 to e6a6bec Compare April 10, 2019 08:25

asbjornu requested a review from dazinator April 10, 2019 08:36

stazz added 2 commits May 8, 2019 12:22

Resolving conflicts related to GitTools#1644 .

66c1435

First modifications for changing from UtilPack.NuGet.MSBuild to NuGet…

9c1fc6c

…Utils.MSBuild.Exec.

stazz added 10 commits May 8, 2019 12:22

Also updated .nuspec file with new dependency, and modified task fact…

7a077a5

…ory name in .props file.

Making GitVersionTask.Tests compilable again.

9c74cd7

Updated to newer NuGetUtils.MSBuild.Exec, which runs on .NET Desktop …

e4ebb13

…successfully now. Also modified the task factory parameter as needed.

Fixing a typo causing NullReferenceException.

956ae14

Updating to newer NuGetUtils.MSBuild.Exec version, which fixes the bu…

2ae8f1a

…g with ITaskItems (seen as String arrays to GitVersionTask).

Updated NuGetUtils.MSBuild.Exec to 2.0.3.

91650a0

Partially changed UtilPack names to NuGetUtils.MSBuild.Exec. Need mor…

42f30a8

…e thorough check later though.

Forgot to update one reference to renamed property.

6b607fa

Updating to newer version of NuGetUtils.MSBuild.Exec in order to cut …

1b8ae10

…down the amount of process invocations used by the task factory.

Updated the NuGetUtils.MSBuild.Exec version to newer, with most recen…

1a09bc3

…t fixes.

asbjornu force-pushed the feature/NuGetUtilsUpdate branch from e6a6bec to 1a09bc3 Compare May 8, 2019 10:23

asbjornu commented May 8, 2019

View reviewed changes

stazz mentioned this pull request May 9, 2019

MSBUILD : Task factory warning NMSBT010: There is a mismatch between SDK NuGet version (4.9.2) and the NuGet version the task factory was compiled against (4.8.0). There might occur some exotic errors. #1554

Closed

stazz mentioned this pull request May 12, 2019

Migrating from UtilPack to NuGetUtils, take two #1677

Merged

asbjornu closed this May 20, 2019

asbjornu deleted the feature/NuGetUtilsUpdate branch May 20, 2019 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NuGetUtils update #1645

NuGetUtils update #1645

asbjornu commented Mar 21, 2019

asbjornu commented Mar 21, 2019

arturcic commented Mar 22, 2019

asbjornu commented Mar 22, 2019

arturcic commented Mar 22, 2019 •

edited

Loading

asbjornu commented Mar 22, 2019

arturcic commented Mar 22, 2019

asbjornu commented Mar 22, 2019

arturcic commented Mar 22, 2019

asbjornu commented Mar 22, 2019

asbjornu commented Mar 23, 2019

asbjornu commented Mar 24, 2019 •

edited

Loading

tpaxatb commented Mar 25, 2019

asbjornu commented Mar 25, 2019

arturcic commented Mar 25, 2019

stazz commented Mar 25, 2019

stazz commented Mar 27, 2019

stazz commented Mar 27, 2019

feitzi commented May 6, 2019

stazz commented May 8, 2019

asbjornu left a comment

asbjornu May 8, 2019

stazz May 8, 2019 •

edited

Loading

asbjornu May 9, 2019

asbjornu May 8, 2019

stazz May 8, 2019

asbjornu May 9, 2019

asbjornu May 8, 2019

stazz May 8, 2019

asbjornu May 8, 2019

stazz May 8, 2019

stazz commented May 8, 2019

asbjornu commented May 9, 2019

stazz commented May 12, 2019

asbjornu commented May 20, 2019


		public class GenerateGitVersionInformation : GitVersionTaskBase
		public static class GenerateGitVersionInformation


		GitVersionInformationFilePath = Path.Combine(workingDirectory, fileName);
		public sealed class Input


		public class GetVersion : GitVersionTaskBase
		public static class GetVersion

NuGetUtils update #1645

NuGetUtils update #1645

Conversation

asbjornu commented Mar 21, 2019

asbjornu commented Mar 21, 2019

arturcic commented Mar 22, 2019

asbjornu commented Mar 22, 2019

arturcic commented Mar 22, 2019 • edited Loading

asbjornu commented Mar 22, 2019

arturcic commented Mar 22, 2019

asbjornu commented Mar 22, 2019

arturcic commented Mar 22, 2019

asbjornu commented Mar 22, 2019

asbjornu commented Mar 23, 2019

asbjornu commented Mar 24, 2019 • edited Loading

tpaxatb commented Mar 25, 2019

asbjornu commented Mar 25, 2019

arturcic commented Mar 25, 2019

stazz commented Mar 25, 2019

stazz commented Mar 27, 2019

stazz commented Mar 27, 2019

feitzi commented May 6, 2019

stazz commented May 8, 2019

asbjornu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stazz May 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stazz commented May 8, 2019

asbjornu commented May 9, 2019

stazz commented May 12, 2019

asbjornu commented May 20, 2019

arturcic commented Mar 22, 2019 •

edited

Loading

asbjornu commented Mar 24, 2019 •

edited

Loading

stazz May 8, 2019 •

edited

Loading