Change cli target detection issue 3705 - #3706

/cc @matthid @atlemann

My deduction in the second commit was apparantly faulty. I'll look into it.

Another way to check for local clitool is like this:
#3672 (comment)

Basically reading the tool-manifest.json file. That would avoid invoking the tool. It's not as bullet proof though.

This has become a bit of a mess due to all the different ways paket has been bootstrapped over time. It would have been nice to just have local or global clitool and ditch the rest, but that will probably break a lot of projects out there.

When adding support for local clitool, we just added all the previous steps as they were after checking for local to avoid breaking things. My first attempt broke global tool support...

I guess it's fine to check for local clitool after all the searching inside .paket folder, but we have to prioritize local clitool before global tool.

@atlemann what are the downsides of that approach? Because if it only supplies false negatives (saying the CLI tool isn't there when it is) we can just add that as an extra step before attempting with dotnet paket --version.

I've removed my second commit, and trying again. Debugging is hard when I can't get it to run locally.

I'm trying to use VS Code on a Windows machine, but I can't seem to run the tests, and I've got all sorts of things that won't load correctly. Any thorough guide on getting a dev environment to run?

When I tried to fix it the last time I was just doing manual testing by changing the paket.Restore.targets file in one of my own projects and setting export PAKET_SKIP_RESTORE_TARGETS=true and installing paket as global and then as local tool etc. and seeing if they all worked.

Don't think there are any downsides by checking the tools-manifest except false negatives as you said. Then running dotnet paket --version could maybe be the last check if none of the others find anything.

I don't think so. I haven't had time to try it, only looked at it on my phone. And I think maybe we should add that parsing of tools.json first to avoid invoking dotnet all the time.

Can someone remind me. What are we doing with the Paket version in the targets file?

Nothing, it's just used as a means to see if the process fails or not. Like, what's the cheapest paket command we can call to see if local tool invocation succeeds.

Yes, IMHO we could replace invoking dotnet paket with checking the manifest file e.g. as shown in that comment I linked to. There might be better ways to check a file for some substring I msbuild though.

And we should update paket.targets to keep them in sync.

Ok, I'll try to sum up status for now:

Primary concerns:

1. Searching for the CLI can be improved by looking at dotnet-tools.json as @atlemann suggested, and I believe that will reduce the times it errors even more - I'll look into it right away.
2. paket.targets should be updated too - I can't find the source for that file though? Can anyone point me towards that?

Secondary concerns:

1. The search order this PR introduces is: "root => tool => CLI => proj style => bootstrapper => global", this keeps CLI before global, but lets local versions in root and tool folder take precedence. That only leaves "proj style / bootstrapper" - if they too should have precedence over CLI, I think a new issue should be created to tackle that. I think the same arguments for changing the order for root/tool could be made for bootstrapper as well. But for now, this PR changes nothing for bootstrapper projecets.
2. Removing the option that changed .dll to force the use of dotnet made the build the fail, so it is obviously being used. I'll try to see where that is, but for now, that can be ignored.

Oh, and forgot another point - is adding "PaketDisableCliTest" something that should be introduced or not? I haven't tested it out yet (I do not have access to the code here that uses paket), and I'm wondering if it is, at all, useful anymore, with the other fixes in place?

Hmm, there's a lot of failures under the "Allowed failures" section. Is that because they are flaky, or?

Well, then I actually think this is a wrap for the main concern.

That leaves:

1. docs - where in the docs does this fit? I think the priority would be important to document? I'll gladly do it, if you have a good suggestion as to where to integrate it? :)
2. "PaketDisableCliTest" - should that be kept or removed? If kept, it should be tested and documented - docs I'll gladly add in the above docs, but I've yet to have testing work for me. :-/
3. paket.targets - where do I find that? If it's long behind the other, I think that should be moved to a separate issue.and the

Am I missing something? :)

paket.targets is only found in:

@atlemann @matthid are we ok with merging this in?

Nope, I'm getting this error while using local tools.

error MSB3073: The command ""paket" restore" exited with code 127.

when setting

<PaketDisableCliTest Condition=" '$(PaketDisableCliTest)' == '' ">True</PaketDisableCliTest>

which means that the dotnet-tools.json check doesn't work.

This is on Ubuntu btw.

@forki ah, my bad, I thought that was for setting up .paket for the paket repo itself. Is that target file what is distributed to Full Framework installations using the old proj format then?

@atlemann well that proves the switch works at least. ;) So in your environment, you do have the dotnet-tools.json placed here relative to the target file: ..\.config\.dotnet-tools.json - plus it contains the word "paket" somewhere in it? Because that's what the test is supposed to do at the moment. :)

Yes, there's where I have it and it contains paket. Probably just a silly little thing.

@atlemann it was indeed... Can you try it out again with the latest change? :)

@JohnyWS Seems to work now.

As for paket.targets, we just copied over the SetPaketCommand target the last time, so I guess it should just be updated with the same changes you made here.

@atlemann I pushed the changes to both target files in the .paket folder now. Can you double check it still works for you? :)

@forki and @atlemann : what about docs - or can we postpone that part? Would really like this change, because everytime someone updates a package, it overrides our local hack... :)

I'm getting this for some reason:

.paket\paket.targets(45,11): error MSB4100: Expected "!$(PaketDisableCliTest)" to evaluate to a boolean instead of "!", in condition " '$(PaketExePath)' == '' AND !$(PaketDisableCliTest) AND !$(_ConfigContainsPaket)".

Hmm...seems to be an issue when building from VisualStudio, but doing dotnet build from the command line doesn't give that error.

A solution with only new SDK proj files which uses paket.Restore.targets works both with CLI dotnet build and from VisualStudio build, so seems to be something weird with old SDK format.

@atlemann Hmm, I seem to be needed a lot more details to reproduce it exactly.

The case I've tried:

1. .NET 4.7.2 project with auto-restore on and old proj format
2. built from commandline with msbuild with or without full git clean -xfd
3. build from Visual Studio with or without full git clean -xfd

I could not make it fail like you did. Any details I'm missing?

Hmmm... I just copy-pasted the raw paket.targets file from this PR into my solution and built in VS2019. I can try to do a proper clean and try again tomorrow at work.

It seems to work. Not sure what happened yesterday, but I just tried copying it over again today and now it builds.

I believe so yes. Any objections @atlemann ? :)

No, it should be fine.

It would be nice to sync such changes with a fake release, but I have no idea how that would look like in practice...

thanks! it's released

Thanks! I just realized that our team working on this massive solution we have, had this same issue with the build hanging randomly. They just didn't tell me...and it didn't happen for me the few times I have built it lately. Works better now!

hehe. awesome!