search

Albacore / albacore

Albacore is a professional quality suite of Rake tasks for building .NET or Mono based systems.
www.albacorebuild.net
221 stars 64 forks source link

Get latest msbuild from registry, resolves #170 #171

Closed aaronasmith closed 9 years ago

haf commented 9 years ago

@devployment – does this PR work for you as well?

What I'm asking is whether you @aaronasmith think this would work when you don't have VS2015 installed?

devployment commented 9 years ago

@haf I'll check. But I have to install VS15 first as I removed a fairly old CTP recently. But only one reason more to finally do so. But I don't see any reason why it should not work generally. Will give it a try today evening.

The only thing I "don't like" is the implicit return of nil for non Win32.windows environments. Generally I like to be explicit. But as the return value for Win32 environments is already kind of implicit on its own it should be ok as it is.

But I think that's more a matter of personal preference of the writer. If mainly using "traditional" languages explicit would be preferred. More ruby'isch would be as it is I think. At the end @haf should decide what he likes the most inside "his" code. :)

haf commented 9 years ago

@devployment You have a point about the nil. I think it can be made even nicer though:

      def heuristic_executable
+       return nil unless ::Rake::Win32.windows?
-       if ::Rake::Win32.windows?
        require 'win32/registry'
... stuff ...
-       end

Since it always returns nil for non-windows. That way we don't get as deep nesting either.

@devployment You did note that I asked you about having VS2015 NOT installed, right?

devployment commented 9 years ago

:+1:

haf commented 9 years ago

@aaronasmith Could you make that final change and then we'll hang back for devployment to test without VS2015 installed?

aaronasmith commented 9 years ago

I do believe this will work if VS 2015 isn't installed. I have no way of verifying that though. It just picks the highest versioned MSBuild instead of looping through the hardcoded list that existed before. I will make your suggested change and update the pull request.

aaronasmith commented 9 years ago

@devployment Are you checking if it works BEFORE you install VS 2015? You mentioned you needed to install it and what @haf wanted to know was if it worked with older versions I believe.

devployment commented 9 years ago

I'll test it the current machine where i removed the CTP. Then I will install 15 again to see what happens. Maybe i give it a spin on a VM as well to start fresh with only 13 installed. I think I have VMs laying around.

devployment commented 9 years ago

@haf I ran my local Albacore powered build now before installing VS15 and after. Both times the most current msbuild was picked. All looks good. Haven't tested it on a fresh machine or other scenarios as it looks like my VMs are gone. But as @aaronasmith already said. It's picking the highest number. So not that much that could go wrong.

But feel free to quote us if there pops up a bug related to this and something went wrong. ;)

haf commented 9 years ago

Thanks @aaronasmith for writing the code and @devployment for verifying :) Team-work ftw!

haf commented 9 years ago

Released v2.5.1 with this in it.