Exclude package versions from tool package folders #1410

thomaslevesque · 2018-08-17T17:26:52Z

Just an idea, feel free to close if you don't like it...

This will make updates easier as we won't have to update the paths in csx scripts.

Small gotcha: you have to remove the packages already installed first. Even with -ExcludeVersion, nuget install won't reinstall package Foo 1.0.0 if it already exists in folder Foo.1.0.0 (which feels like a bug to me) (solved by checking if packages.config's SHA1 has changed since last run)

thomaslevesque · 2018-08-17T17:27:58Z

tools/run-csx.cmd

@@ -26,10 +26,10 @@ if not exist %TOOLS_DIR%\.nuget md %TOOLS_DIR%\.nuget
 copy /Y %NUGET_CACHE_DIR%\NuGet.exe %TOOLS_DIR%\.nuget\NuGet.exe > nul

 rem restore packages for build script
-%TOOLS_DIR%\.nuget\NuGet.exe restore %TOOLS_DIR%\packages.config -PackagesDirectory %TOOLS_DIR%\packages -Verbosity quiet
+%TOOLS_DIR%\.nuget\NuGet.exe install %TOOLS_DIR%\packages.config -OutputDirectory %TOOLS_DIR%\packages -Verbosity quiet -ExcludeVersion


nuget install on a packages.config basically does the same thing as nuget restore, but has the -ExcludeVersion switch

thomaslevesque · 2018-08-17T17:32:45Z

Actually I just realized something... If we update the version in packages.config, NuGet won't know that the currently installed version isn't the right one, so it won't reinstall it. I have an idea to work around this, stay tuned.

blairconrad · 2018-08-17T17:40:37Z

I see a failing build that is not your fault:

https://ci.appveyor.com/project/FakeItEasy/fakeiteasy/build/633-cyahcldk#L342

FakeItEasy.Specs.GenericDummyCreationSpecs.ClassWhoseLongerConstructorThrowsCreation() [10] And the longer constructor was only attempted once [FAIL]

The actual count was 2. I suspect this is a race condition, which you predicted, @thomaslevesque. I'll investigate as time allows.

thomaslevesque · 2018-08-17T18:17:29Z

Actually I just realized something... If we update the version in packages.config, NuGet won't know that the currently installed version isn't the right one, so it won't reinstall it. I have an idea to work around this, stay tuned.

Here's my idea:

compute a hash of the packages.config file
compare with a previously stored hash
if the hashes are different, delete everything in the packages directory
store the new hash in a file

(it's the approach used by Cake)

What do you think?

blairconrad · 2018-08-17T18:36:18Z

That sounds pretty good, @thomaslevesque.

thomaslevesque · 2018-08-18T09:45:54Z

Would you mind if I converted run-csx.cmd to PowerShell? PowerShell (4+) has a Get-FileHash command that would be very helpful

thomaslevesque · 2018-08-18T11:05:30Z

All done, tell me what you think!

blairconrad

I like it, @thomaslevesque.
I was going to suggest a more granular approach to deleting the old packages, but they're probably just sitting in the NuGet cache anyhow, so I think this is the right approach.

I did make some minor comments, but would not hold up the merge for them, nor for this one:
now that we have PowerShell, I notice the various comments in the file and in some cases think that the blocks of functionality could easily be extracted into their own functions, as part of my motto "if it's worth commenting, it could be a function". Do you think there's value in that? If not, say the word, update the title (to not be WIP) and I'll merge.

blairconrad · 2018-08-18T17:33:12Z

tools/run-csx.ps1

+& $NuGetExe install $PackagesConfig -OutputDirectory $PackagesDir -Verbosity quiet -ExcludeVersion
+
+# run build script
+$CsiExe = "$ToolsDir\packages\Microsoft.Net.Compilers\tools\csi.exe"


It's a small thing, but $PackagesDir instead of $ToolsDir\package?

blairconrad · 2018-08-18T17:35:40Z

tools/run-csx.ps1

+$PackagesHash = (Get-FileHash -Algorithm SHA1 -Path $PackagesConfig).Hash
+# if packages.config has changed, clear packages and store new hash
+if ($PackagesHash -ne $PreviousPackagesHash) {
+    Write-Host "Packages.config has changed, reinstalling packages..."


I'd consider $PackagesConfig instead of "Packages.config", unless you want to hide the path (which is fine)

thomaslevesque · 2018-08-18T18:55:21Z

now that we have PowerShell, I notice the various comments in the file and in some cases think that the blocks of functionality could easily be extracted into their own functions, as part of my motto "if it's worth commenting, it could be a function". Do you think there's value in that?

Sure, I'll do it

thomaslevesque · 2018-08-19T11:20:32Z

tools/run-csx.ps1

+Push-Location $RepoRoot
+
+DownloadNuGetToCache
+CopyNuGetLocally


I assume there's a good reason for copying nuget in a local folder, but I don't remember what it is... Why don't we use it directly from the cache directory? Or why don't we download it directly to the local directory?
I think the local directory could act as the cache; a build script has no business writing to the user's AppData folder.

Yeah, I stumble over this as well. I think @adamralph originated the behaviour or imported from somewhere else.
I wouldn't mind if either the caching or the copying went away (I don't mind the build script writing to the AppData folder, but don't mind it not doing it either).
The current system does give a way to always have the correct nuget in the local directory (because the cache directory is versioned) without downloading it all the time or having the rest of the build system need to know what the version is.
I guess it also allows one to switch between (already cached) versions while disconnected. but that's a small concern.

I don't even mind the idea of committing the nuget exe, if we want.

You might find it simpler to do away with scripting completely, as I've done with xbehave and @blairconrad has done with SelfInitializingFakes. It allows you to drop nuget.exe, csi.exe, packages.config, etc. All you need is a csproj for package references and a Program.Main for your build "script". You can then invoke simply with dotnet run.

Yes, it might make things easier...

Actually, a mixed VB/C# solution isn't a use-case either, since an analyzer is installed in a project, which can't be mixed language.

No, but the meta-package does provide a convenient way to update both C# and VB versions at once. For me, it's not enough of an advantage to continue with it, though.

Another option is to use dotnet script instead of manually invoking csi.exe. It natively supports NuGet references. It requires .NET Core SDK 2.1+, though, since it's a global tool.

dotnet script also requires bootstrapping, or listing as another pre-requisite. The beauty of the dotnet run approach is that nothing else is required other than the dotnet SDK, which is already a pre-requisite.

Also, you get full tooling support, in all IDEs, since it's just a regular C# project. No special scripting plug-ins, etc. required.

That's a good point!

blairconrad · 2018-08-19T16:54:14Z

@thomaslevesque, thanks for the changes. It looks great. I'm happy to merge after squash. And continue to explore the "cached and copied nuget" either here or elsewhere.

thomaslevesque · 2018-08-19T21:40:04Z

And continue to explore the "cached and copied nuget" either here or elsewhere.

👍

blairconrad · 2018-08-19T21:50:04Z

Nice job, @thomaslevesque. Thanks.

thomaslevesque · 2018-08-19T21:51:54Z

Thanks for the merge, and the comments!

adamralph · 2018-08-22T10:49:44Z

tools/build.csx

-#r "packages/Bullseye.1.1.0-rc.2/lib/netstandard2.0/Bullseye.dll"
-#r "packages/SimpleExec.2.2.0/lib/netstandard2.0/SimpleExec.dll"
+#r "packages/Bullseye/lib/netstandard2.0/Bullseye.dll"
+#r "packages/SimpleExec/lib/netstandard1.3/SimpleExec.dll"


@FakeItEasy/owners was the change back to netstandard1.3 intentional? The restoration graph is much lighter if you use netstandard2.0.

@adamralph no, it's not intentional. I must have done it when I resolved a conflict with #1409

Whoops. That had caught my eye initially and I forgot about it in the other conversation (none of which was as important as that).

blairconrad · 2018-09-04T10:37:07Z

This change has been released as part of FakeItEasy 4.8.1.

blairconrad · 2018-09-04T10:38:40Z

This change has been released as part of FakeItEasy 4.8.1.

thomaslevesque commented Aug 17, 2018

View reviewed changes

thomaslevesque changed the title ~~Exclude package versions from tool package folders~~ [DO NOT MERGE] Exclude package versions from tool package folders Aug 17, 2018

thomaslevesque force-pushed the exclude-package-version branch from 855c106 to fc15b04 Compare August 17, 2018 17:34

blairconrad approved these changes Aug 18, 2018

View reviewed changes

blairconrad mentioned this pull request Aug 18, 2018

Serialize dummy/fake creation in specs that count constructor calls #1412

Merged

thomaslevesque changed the title ~~[DO NOT MERGE] Exclude package versions from tool package folders~~ Exclude package versions from tool package folders Aug 19, 2018

thomaslevesque commented Aug 19, 2018

View reviewed changes

thomaslevesque force-pushed the exclude-package-version branch from 5f9b233 to dc6ab49 Compare August 19, 2018 11:24

Exclude tool package versions

cfeb094

thomaslevesque force-pushed the exclude-package-version branch from dc6ab49 to b50be05 Compare August 19, 2018 11:47

thomaslevesque added 3 commits August 19, 2018 23:41

Convert run-csx.cmd to PowerShell

5434314

Reinstall packages if packages.config has changed

c508346

Refactor run-csx.ps1 to use functions

e4b5235

thomaslevesque force-pushed the exclude-package-version branch from b50be05 to e4b5235 Compare August 19, 2018 21:41

blairconrad merged commit 169b809 into FakeItEasy:develop Aug 19, 2018

blairconrad added this to the vNext milestone Aug 19, 2018

thomaslevesque deleted the exclude-package-version branch August 19, 2018 21:51

adamralph reviewed Aug 22, 2018

View reviewed changes

thomaslevesque mentioned this pull request Aug 23, 2018

Consider replacing the csx scripts by full-fledged csprojs #1413

Closed

thomaslevesque mentioned this pull request Aug 28, 2018

Convert csx scripts to csproj #1414

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude package versions from tool package folders #1410

Exclude package versions from tool package folders #1410

thomaslevesque commented Aug 17, 2018 •

edited

thomaslevesque Aug 17, 2018

thomaslevesque commented Aug 17, 2018

blairconrad commented Aug 17, 2018 •

edited

thomaslevesque commented Aug 17, 2018

blairconrad commented Aug 17, 2018

thomaslevesque commented Aug 18, 2018

thomaslevesque commented Aug 18, 2018

blairconrad left a comment •

edited

blairconrad Aug 18, 2018

blairconrad Aug 18, 2018

thomaslevesque commented Aug 18, 2018

thomaslevesque Aug 19, 2018

blairconrad Aug 19, 2018

blairconrad Aug 19, 2018

adamralph Aug 22, 2018

thomaslevesque Aug 22, 2018

adamralph Aug 22, 2018

blairconrad Aug 22, 2018

thomaslevesque Aug 23, 2018

adamralph Aug 23, 2018

thomaslevesque Aug 23, 2018

blairconrad commented Aug 19, 2018

thomaslevesque commented Aug 19, 2018

blairconrad commented Aug 19, 2018

thomaslevesque commented Aug 19, 2018

adamralph Aug 22, 2018

thomaslevesque Aug 22, 2018

blairconrad Aug 22, 2018

blairconrad commented Sep 4, 2018

blairconrad commented Sep 4, 2018

Exclude package versions from tool package folders #1410

Exclude package versions from tool package folders #1410

Conversation

thomaslevesque commented Aug 17, 2018 • edited

Choose a reason for hiding this comment

thomaslevesque commented Aug 17, 2018

blairconrad commented Aug 17, 2018 • edited

thomaslevesque commented Aug 17, 2018

blairconrad commented Aug 17, 2018

thomaslevesque commented Aug 18, 2018

thomaslevesque commented Aug 18, 2018

blairconrad left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaslevesque commented Aug 18, 2018

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

Choose a reason for hiding this comment

blairconrad commented Aug 19, 2018

thomaslevesque commented Aug 19, 2018

blairconrad commented Aug 19, 2018

thomaslevesque commented Aug 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blairconrad commented Sep 4, 2018

blairconrad commented Sep 4, 2018

thomaslevesque commented Aug 17, 2018 •

edited

blairconrad commented Aug 17, 2018 •

edited

blairconrad left a comment •

edited