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

Refactor ICakeEnvironment #502

Closed
patriksvensson opened this issue Nov 7, 2015 · 7 comments
Closed

Refactor ICakeEnvironment #502

patriksvensson opened this issue Nov 7, 2015 · 7 comments
Assignees
Milestone

Comments

@patriksvensson
Copy link
Member

Right now, ICakeEnvironment is kind of a mess, mixing properties and methods for things.

I'm also not a fan of IsUnix-property which I think could be more fine grained i.e. we could add a OperatingSystem enum that gives more information such as Windows, OSX, Linux etc. We could even make this a flags enum and add additional more finegrained information such as what operating system, but not sure if that would be useful.

I would suggest that we make some changes.

It would make sense to introduce something like IPlatform that could keep information about OS and hardware. The ICakeEnvironment have a tendency to become a dumping ground for everything environment related and this would mean that we would keep platform specific stuff isolated.

  • Move bool Is64BitOperativeSystem() to IPlatform
  • Move bool IsUnix() to IPlatform
  • Change DirectoryPath GetApplicationRoot(); to DirectoryPath ApplicationRoot { get; }
  • Add IPlatform Platform { get; }
public interface IPlatform
{
  OperatingSystem OperatingSystem { get; }
  bool Is64Bit { get; }
  bool IsUnix { get; } // Could be an extension method for IPlatform
}
public enum OperatingSystem
{
  Unknown = 0,
  Windows = 1,
  Linux = 2,
  OSX = 3
}

Not sure if we should expose the enum directly, or expose a class that
contains information about the operating system (type/flavor, name, version) etc.
Not sure if this information would be of interest to Cake users.

NOTE: Since this is a breaking change, we wouldn't actually remove the methods but mark them as obsolete and remove them sometime in the future.

I will glady do this changes if you let me 😄

@gep13
Copy link
Member

gep13 commented Nov 8, 2015

This sounds like a good idea to me, your reasoning seems sound to me.

Agree with the breaking change, and would think this would bump to 0.7.0, not as a patch version.

@patriksvensson patriksvensson added this to the v0.7.0 milestone Nov 9, 2015
@patriksvensson patriksvensson modified the milestones: v0.7.0, v0.8.0 Dec 14, 2015
@khellang
Copy link
Contributor

It might be worth looking at the CoreFX APIs to get this information. A lot of the APIs that are used currently doesn't existing in this new world.

This might be a place where you'd have to #ifdef and have two implementations of IPlatform(Information?), one for full CLR and one for core...

@devlead
Copy link
Member

devlead commented Dec 28, 2015

System.Runtime.InteropServices and many others are on NuGet so those bridge the gap between Core & Standard CLR features sets.

Probably worth obsoleting stuff and then later remove if no one compains 😉

@khellang
Copy link
Contributor

System.Runtime.InteropServices and many others are on NuGet so those bridge the gap between Core & Standard CLR features sets.

Sure, if you don't mind that dependency on full CLR 😄

@devlead
Copy link
Member

devlead commented Dec 28, 2015

AFAIK you'd on .NETCore 5.0 add dependencies to

  • System.Runtime (≥ 4.0.0)
  • System.Reflection (≥ 4.0.0)
  • System.Reflection.Primitives (≥ 4.0.0)
  • System.Runtime.Handles (≥ 4.0.0)

@patriksvensson patriksvensson modified the milestones: v0.8.0, v0.9.0 Jan 9, 2016
@gep13 gep13 added the Roadmap label Jan 18, 2016
@gep13 gep13 modified the milestones: v0.10.0, v0.9.0 Jan 18, 2016
@gep13 gep13 modified the milestone: v0.10.0 Mar 9, 2016
@devlead
Copy link
Member

devlead commented May 25, 2016

As discussed on Gitter

For child processes we could probably add an environment dictionary property to ToolSetting and ProcessSettings that optionally passed along to ProcessStartInfo.Environment

Something we could look at during the ICakeEnvironment refactoring.

@gep13
Copy link
Member

gep13 commented May 25, 2016

A related issue which should be tackled at the same time... #902

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants