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

Bootstrapper magic mode enhancements #1983

Merged
merged 13 commits into from Oct 28, 2016

Conversation

Projects
None yet
3 participants
@vbfox
Contributor

vbfox commented Oct 26, 2016

Quite a few changes to the boostrapper, mainly related to magic mode :

  • Add tests around the fact that MaxFileAgeInMinutes shouldn't be auto set in magic mode when a version or prerelease is specified.

  • Clean the argument parser code and clearly show in what order we apply our different sources of options

  • Add a new source of bootstrapper options (Magic mode or not, but especially useful in magic mode) : The paket.dependencies file. The first comment starting with regex (#|//)(\s*)bootstrapper(\s*): ( is taken into account for that :

    # bootstrapper: 3.24.1 --prefer-nuget
    
    source https://api.nuget.org/v3/index.json
    nuget FAKE
    nuget FSharp.Core ~> 4
    nuget FSharpLint.MSBuild
    
  • The new order of option sources is : AppSettings, paket.dependencies, Environment variables, Command line (Processing order in code so, from less to more important. If something is on the command line it'll override any other source for example)

  • There is a new variant of "silent" mode that displays only errors. It can't be triggered from command line but the magic bootstrapper uses it (Otherwise it display nothing in case of error and that's confusing)

  • Known nuget download errors are now displayed with a better explanation

  • MSBuild targets file can now work with a paket.exe (Most often the bootstrapper in magic mode) at the root instead of in .paket

  • The directory / file proxy have been merged into a filesystem proxy and this proxy gained a few methods for better magic mode tests.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 27, 2016

Member

I really like the idea of specifying a needed paket version in the deps file. That is a very often requested feature (IIRC we already have an issue for that but I can't find it). But I don't think we should make that a "comment". It's too important. How about this:

version 3.24.1 --prefer-nuget

source https://api.nuget.org/v3/index.json
nuget FAKE
nuget FSharp.Core ~> 4
nuget FSharpLint.MSBuild

or something like that. It would also be interesting to specify version ranges for paket. Someting like version >= 3.23

Member

forki commented Oct 27, 2016

I really like the idea of specifying a needed paket version in the deps file. That is a very often requested feature (IIRC we already have an issue for that but I can't find it). But I don't think we should make that a "comment". It's too important. How about this:

version 3.24.1 --prefer-nuget

source https://api.nuget.org/v3/index.json
nuget FAKE
nuget FSharp.Core ~> 4
nuget FSharpLint.MSBuild

or something like that. It would also be interesting to specify version ranges for paket. Someting like version >= 3.23

@agross

This comment has been minimized.

Show comment
Hide comment
@agross

agross Oct 27, 2016

Contributor

It'd be great if the version statement would be optional to always get latest (within MaxFileAgeInMinutes).

Contributor

agross commented Oct 27, 2016

It'd be great if the version statement would be optional to always get latest (within MaxFileAgeInMinutes).

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 27, 2016

Member

yes it needs to be optional - also for backwards compat

Member

forki commented Oct 27, 2016

yes it needs to be optional - also for backwards compat

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 27, 2016

Contributor

Using a normal line instead of a comment is conceptually easy and will look cleaner but I need to test compatibility of paket parser itself. I'll add that if it works well.

Ranges are possible but we'll start to duplicate some of the version check logic of paket itself in the bootstrapper. Maybe for a later PR ? 🦊

Contributor

vbfox commented Oct 27, 2016

Using a normal line instead of a comment is conceptually easy and will look cleaner but I need to test compatibility of paket parser itself. I'll add that if it works well.

Ranges are possible but we'll start to duplicate some of the version check logic of paket itself in the bootstrapper. Maybe for a later PR ? 🦊

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 27, 2016

Member

Absolutely. We don't need that now. Pinning a paket version is already awesome

Member

forki commented Oct 27, 2016

Absolutely. We don't need that now. Pinning a paket version is already awesome

vbfox added some commits Oct 24, 2016

Take magic bootstrapper arguments from dependencies file
The bootstrapper in magic mode can now take it's arguments from a special
line in 'paket.dependencies' the syntax is:

    # bootstrapper: <arguments>
Add more bootstrapper tests
Also merge DirectoryProxy and FileProxy to add path methods. The new proxy
is named a FileSystemProxy.
MSBuild targets now work with paket.exe in root directory
It's now possible to do that with the magic mode and as disussed in #1961 the targets file should support it
Replace '#boostrapper:' with 'version'
The boostrapper configuration line in paket.depdendencies now look
like a normal directive instead of being a comment.

Paket parser needed a little change to skip such lines instead of
complaining.
@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 27, 2016

Contributor

New version with the syntax suggested by @forki :

version 3.24.1 --prefer-nuget

source https://api.nuget.org/v3/index.json
nuget FAKE
nuget FSharp.Core ~> 4
nuget FSharpLint.MSBuild
Contributor

vbfox commented Oct 27, 2016

New version with the syntax suggested by @forki :

version 3.24.1 --prefer-nuget

source https://api.nuget.org/v3/index.json
nuget FAKE
nuget FSharp.Core ~> 4
nuget FSharpLint.MSBuild
@agross

This comment has been minimized.

Show comment
Hide comment
@agross

agross Oct 27, 2016

Contributor

Sorry if I didn't get it, but is --prefer-nuget passed in verbatim passed to the bootstrapping that then is paket.exe?

​Alex

http://therightstuff.de/
Tiny phone, tiny mail​

On Oct 27, 2016, 22:16, at 22:16, Julien Roncaglia notifications@github.com wrote:

New version with the syntax suggested by @forki :

version 3.24.1 --prefer-nuget

source https://api.nuget.org/v3/index.json
nuget FAKE
nuget FSharp.Core ~> 4
nuget FSharpLint.MSBuild

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#1983 (comment)

Contributor

agross commented Oct 27, 2016

Sorry if I didn't get it, but is --prefer-nuget passed in verbatim passed to the bootstrapping that then is paket.exe?

​Alex

http://therightstuff.de/
Tiny phone, tiny mail​

On Oct 27, 2016, 22:16, at 22:16, Julien Roncaglia notifications@github.com wrote:

New version with the syntax suggested by @forki :

version 3.24.1 --prefer-nuget

source https://api.nuget.org/v3/index.json
nuget FAKE
nuget FSharp.Core ~> 4
nuget FSharpLint.MSBuild

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#1983 (comment)

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 27, 2016

Contributor

@agross The feature is actually completely independent of the "magic" mode where paket.exe is actually the bootstrapper. It's very useful in this mode but could have existed independently of it and would work without it.

What happens is :

  • paket.dependencies contains version 1.2.3.4 --prefer-nuget
  • User run bootstrapper
  • The bootstrapper start and read it's options from all it's sources, including the version line in paket.dependencies, merging them into one set of options. If neither environment variables or the command line specifies a version 1.2.3.4 is used. Prefer nuget is always used as there is no way to disable a flag.
  • It then download paket or not as usual (If already present on that version or not)
  • User start paket
  • Paket read the same paket.dependencies file completely ignoring the version marker

Where it really shine is when you combine this, the --run parameter and the magic mode (That is nothing else than an implicit --run parameter depending on the name of the executable) :

  • paket.dependencies contains version 1.2.3.4
  • User run paket.exe restore
  • The bootstrapper start and read it's options from all it's sources, including the version line in paket.dependencies. If environment variables doesn't specify a version 1.2.3.4 is used.
  • It then download paket or not as usual and place it in the temp folder (that's activated by "magic" mode)
  • Paket is started with the "restore" parameter.
  • Paket read the same paket.dependencies file completely ignoring the version marker
Contributor

vbfox commented Oct 27, 2016

@agross The feature is actually completely independent of the "magic" mode where paket.exe is actually the bootstrapper. It's very useful in this mode but could have existed independently of it and would work without it.

What happens is :

  • paket.dependencies contains version 1.2.3.4 --prefer-nuget
  • User run bootstrapper
  • The bootstrapper start and read it's options from all it's sources, including the version line in paket.dependencies, merging them into one set of options. If neither environment variables or the command line specifies a version 1.2.3.4 is used. Prefer nuget is always used as there is no way to disable a flag.
  • It then download paket or not as usual (If already present on that version or not)
  • User start paket
  • Paket read the same paket.dependencies file completely ignoring the version marker

Where it really shine is when you combine this, the --run parameter and the magic mode (That is nothing else than an implicit --run parameter depending on the name of the executable) :

  • paket.dependencies contains version 1.2.3.4
  • User run paket.exe restore
  • The bootstrapper start and read it's options from all it's sources, including the version line in paket.dependencies. If environment variables doesn't specify a version 1.2.3.4 is used.
  • It then download paket or not as usual and place it in the temp folder (that's activated by "magic" mode)
  • Paket is started with the "restore" parameter.
  • Paket read the same paket.dependencies file completely ignoring the version marker
@agross

This comment has been minimized.

Show comment
Hide comment
@agross

agross Oct 27, 2016

Contributor

Hm, if --prefer-nuget is always enabled, why do I need to specify it? It looks like a command line parameter to me.

Say for example paket.bootstrapper.exe v2017 supports --fallback <url>. Would it be possible to have something like this in paket.dependencies?

version 1.2.3 --prefer-nuget --fallback http://example.com/paket.exe
Contributor

agross commented Oct 27, 2016

Hm, if --prefer-nuget is always enabled, why do I need to specify it? It looks like a command line parameter to me.

Say for example paket.bootstrapper.exe v2017 supports --fallback <url>. Would it be possible to have something like this in paket.dependencies?

version 1.2.3 --prefer-nuget --fallback http://example.com/paket.exe
@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 28, 2016

Contributor

Hm, if --prefer-nuget is always enabled

It's not always enabled, what's after version is the same as the command line of the bootstrapper and just treated as another option source.
I was noting that it was always enabled there because the bootstrapper doesn't support negative options (Like an hypothetical --no-prefer-nuget) so once one option source specify that a Boolean is different from it's default there is no way to go back so if paket.dependencies specifies --prefer-nuget there is no way to use github from the command line. (But it's not specific to this change, it already happened before between appsettings and the command line)

Say for example paket.bootstrapper.exe v2017 supports --fallback <url>. Would it be possible to have something like this in paket.dependencies?

Yes, see the code of ArgumentParser.cs in this PR, the line in paket.dependencies is parsed using the new FillNonRunOptionsFromArguments and it's the same function that parses the normal command line. The only difference is that --run isn't possible for the rest of the arguments it's the same code.

The only real complexity is in correctly splitting the arguments into a string[] but that's the role of WindowsProcessArguments.Parse that implements how windows processes normally do it (The OS doesn't provide it and anyway as we want paket.dependencies to be shareable between OSes we must fix ourselves on one algorithm)

Contributor

vbfox commented Oct 28, 2016

Hm, if --prefer-nuget is always enabled

It's not always enabled, what's after version is the same as the command line of the bootstrapper and just treated as another option source.
I was noting that it was always enabled there because the bootstrapper doesn't support negative options (Like an hypothetical --no-prefer-nuget) so once one option source specify that a Boolean is different from it's default there is no way to go back so if paket.dependencies specifies --prefer-nuget there is no way to use github from the command line. (But it's not specific to this change, it already happened before between appsettings and the command line)

Say for example paket.bootstrapper.exe v2017 supports --fallback <url>. Would it be possible to have something like this in paket.dependencies?

Yes, see the code of ArgumentParser.cs in this PR, the line in paket.dependencies is parsed using the new FillNonRunOptionsFromArguments and it's the same function that parses the normal command line. The only difference is that --run isn't possible for the rest of the arguments it's the same code.

The only real complexity is in correctly splitting the arguments into a string[] but that's the role of WindowsProcessArguments.Parse that implements how windows processes normally do it (The OS doesn't provide it and anyway as we want paket.dependencies to be shareable between OSes we must fix ourselves on one algorithm)

@forki forki merged commit 82dc23c into fsprojects:master Oct 28, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@agross

This comment has been minimized.

Show comment
Hide comment
@agross

agross Oct 28, 2016

Contributor

@vbfox Alright, now I got it. Thanks!

Contributor

agross commented Oct 28, 2016

@vbfox Alright, now I got it. Thanks!

@forki

This comment has been minimized.

Show comment
Hide comment
@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 28, 2016

Contributor

Yes 👍

The text could be a little clearer in specifying that the version can be the special "prerelease" string but maybe we don't want to incite users to always get prereleases 😁

Contributor

vbfox commented Oct 28, 2016

Yes 👍

The text could be a little clearer in specifying that the version can be the special "prerelease" string but maybe we don't want to incite users to always get prereleases 😁

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 28, 2016

Member

please send PRs to make the docs graet again

2016-10-28 15:07 GMT+02:00 Julien Roncaglia notifications@github.com:

Yes 👍

Maybe the text could be a little clearer in specifying that the version
can be the special "prerelease" string but maybe we don't want to incite
users to always get prereleases 😁


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1983 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNBk0aiQEvA-MMfg5C483V2g5HDPoks5q4fOCgaJpZM4Khssv
.

Member

forki commented Oct 28, 2016

please send PRs to make the docs graet again

2016-10-28 15:07 GMT+02:00 Julien Roncaglia notifications@github.com:

Yes 👍

Maybe the text could be a little clearer in specifying that the version
can be the special "prerelease" string but maybe we don't want to incite
users to always get prereleases 😁


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#1983 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNBk0aiQEvA-MMfg5C483V2g5HDPoks5q4fOCgaJpZM4Khssv
.

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 28, 2016

Contributor

Trump

Contributor

vbfox commented Oct 28, 2016

Trump

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