GeezerVison

in the migrate module, can you provide some output/feedback to the user what has happened?

Regarding migration feedback, I initially wanted to do that, but MEF got the better of me and I couldn't, for the life of me, work out how to get an ILog or whatnot into the module. Or should it be IConsole?

But anyway, I'm really doubting whether migrate should indeed be a module. See https://github.com/adamralph/scriptcs/commit/f452d43d6aabd165fe3bb9d28214d062f5193615#commitcomment-8926049

I don't think you can constructor inject into modules at the moment since it uses different container

I guess you could pass ILog or IConsole instance through ModuleConfiguration

Regarding your comment, I am on the fence here, but if it's a module then it should be an external tool too, an opt-in, and not belong to a core. Each module is an extra cost to load, so I don't want it to be forced upon me if I don't need it.

If I recall the reason for not introducing it as a switch was to not have a switch that would one day be taken away, but I am not sure how valid of an argument this is.

@filipw thanks for the feedback.

@scriptcs/core any other opinions on module vs command for migration?

Why can't you constrictor inject? MEF support this.
On Tue, Dec 23, 2014 at 3:51 AM Adam Ralph [email protected] wrote:

@filipw https://github.com/filipw thanks for the feedback.

@scriptcs/core https://github.com/orgs/scriptcs/teams/core any other
opinions on module vs command for migration?

—
Reply to this email directly or view it on GitHub
#868 (comment).

I've no idea 😝, but if we decide that migrate should be a command then the point is moot, at least in this case.

@scriptcs/core any other opinions on migrate as command or a module?

If a module, then we may need to do something about the UX.

Currently our command line args are mix of commands disguised as options and actual options (we should really consider implementing #175) :

Commands

• run a script: implicit by presence of script name
• start REPL: implicit by absence of script name or -repl when other args are present (this is confusing but is addressed by Default to REPL when other options are passed #869 which is currently also solved by this PR)
• show help -help
• install package -install
• save packages.config file -save
• clean installed packages -clean
• show version -version

Options

• drop into REPL after running script file -repl
• emit PDB symbols -debug
• assembly in memory or filesystem -cache
• log level -loglevel
• install package globally -global
• allow pre-release packages -allowprerelease
• watch script file for changes -watch
• module to load -modules

With the current design, the option to load the migrate module is just that, an option. The command to be performed will have to come from one of the listed commands above. With the change to address #869, typing scriptcs -m migrate will work, but will drop the user into the REPL afterwards. Typing scriptcs foo.csx -m migrate will also work but will obviously also run the script. There is no way to "just migrate", which I'm quite sure I would want as a user so I could assess the outcome before going ahead and trying to execute scripts or REPL. Of course, someone could type something like scriptcs -m migrate -version but that's just weird. Dropping into the REPL isn't the end of the world, but it is odd behaviour when all I want to do is migrate.

OK, I decided to make migration a command. There were just too many outstanding issues and questions around 'modules as commands' and making it a real command turned out to be far more straightforward and less disruptive. Yes, we may take away the migrate command somewhere down the line but I don't consider this a problem.

This PR is far more straightforward now and the commit stream should be self explanatory. It also includes a full acceptance test for migration. The fix for #869 is no longer included here since it is irrelevant. I will send a separate PR for that issue.

Builds are green. This PR is good to go again.

Needs a rebase 😄

Done 😃

How will people know that they need to do this once their scripts break? Docs? Could we detect and run the migration if needed? Or is it too hard to detect? I.e. what if the user doesn't want to migrate existing packages.config etc.?

I think we need to shout about it quite loudly, perhaps a post on http://blog.scriptcs.net/ and then we tweet it around etc, and/or cross post on other blogs owned by @scriptcs/core members.

Detection isn't really possible since, in a given folder, we never know whether packages/, packages.config, etc. is meant for scriptcs or regular .net projects.

We need to make it clear in our announcements exactly what scriptcs -migrate will do. In the case of an existing directory where they have a packages.config which they don't want to migrate then I can't see there being a need to do any migration in that directory at all, since that will imply that packages/ etc. is also not meant for scriptcs.net. Anyway, I think the wholesale migrate will solve almost all cases. In edge cases people may have to migrate bits manually.

Alright

Hi @adamralph

@khellang and I were just talking about this and we both share the concern about how this will break customers. I am worried also about the person who falls in between the cracks where the current migrate won't help them.

Imagine I ran scriptcs within my full .NET app folder. Now you are migrating and moving my core app packages.config to the new scriptcs one. That may make scriptcs work but it breaks my .NET app as the packages.config is gone.

Even if migrate worked, it is still ugly and non-intuitive as the user won't get a warning that would lead them to know to do migrate.

So here's my suggestion.

1. We get rid of the migrate command.
2. We automatically detect if there is a packages.config and packages present and there the scriptcs versions are not present. If so then we COPY packages.config and packages to the new folder.
3. We give the user a clear message that details the upgrade.

By doing a copy instead of a move, we don't affect their existing app. Also going forward if they install new packages with scriptcs-install they will automatically go to the right place.

The downside of this approach is the file copy will result in pulling in packages that may not be needed. One can always go manually and clean those out if they care.

Thoughts?

I really like this idea and I think it's better than what we currently have. I'll send a new PR which reverts the last two commits from this PR and adds what you've described.

What about the other artifacts? I think we'll have to do the same for bin/ and nuget.config. I'm not sure about .cache/ since I guess that gets generated at runtime?

@adamralph great.

For .cache it is safe to rename I think.
For nuget.config, copy.
For bin, probably safest to copy just in case.

One remaining question is where to put this code? I can very easily put it in scriptcs.exe, but what about hosting scenarios? Do we want to offer the ability for hosts to perform the same operation? If so, I'm not really sure where it would need to go.

Also, if we do want to expose it for hosting, should it be implicit for anyone using the hosting layer or should it require an explicit invocation?

I have most of the work done and I'll send a PR soon.

The approach I have gone with is as follows:

I've introduced first class types for migration, i.e. ScriptCs.Contracts.IFileSystemMigrator and ScriptCs.FileSystemMigrator (in ScriptCs.Core.dll) and given them the full wire up, i.e. in ScriptServices, etc.

FileSystemMigrator first looks for the existence of any of the new file system artifacts. If it detects at least one artifact then it assumes migration (either automatic or manual and either full or partial) has already been done and it does not attempt any further migration. If no new file artifacts are detected then any legacy file system artifacts are migrated, using the logic described in the above comment by @glennblock.

Each ICommand implementation that needs to do migration before execution, receives an instance of IFileSystemMigrator which it invokes as soon as Execute() is called, before anything else happens.

The commands which perform migration are: script execution, REPL, -install, -save and -clean. The commands which do not perform migration are -help and -version.

Before sending the PR, I'm going to add acceptance tests for -install, -save and -clean to make sure the change doesn't break them.

If anyone has any comments on the approach, now is the time 😉.