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

Use and forward additional nuget arguments #263

Merged
merged 1 commit into from
Dec 4, 2021

Conversation

husk3r
Copy link
Contributor

@husk3r husk3r commented Dec 2, 2021

Hi Oleg,

I've an use case which needs to specify a personal nuget / artifactory respository. Therefore I tried to add the source with:

//css_nuget -ng:"-s https://artifactory.mydomain.com/artifactory/api/nuget/my-nuget-provider" CS-Script

Unfortunately that didn't work out very well. During debugging I discovered that nugetArgs set by string nugetArgs = packageArgs.ArgValue("-ng"); is not used or forwarded to InstallPackage. This patch fixes that one.

In addition two other things came to my mind:

  1. When I call the script by cscs myScript.cs the nuget packages will be downloaded. However, the script fails when css -vs myScript.cs is called and the local nuget cache is empty. It seems that bool suppressDownloading is set with var refPkAsms = this.ResolvePackages(true); //suppressDownloading but I've no idea why we need this flag.

  2. My script I've written is placed in a git repository which has it's own NuGet.config. This configuration defines also my private artifactory. So the assumption was originally that the script is this configuration too, since it is placed in a parent directory. But the generated project is placed in %temp%. I could be a good idea to copy the NuGet.config to the %temp directory together with the temporary build.csproj.

So far,
Florian

@oleg-shilo oleg-shilo merged commit 2d3cf8a into oleg-shilo:master Dec 4, 2021
@oleg-shilo
Copy link
Owner

Excellent. Thank you.

Answering your questions:

  1. suppressDownloading is required for some cases when performance even more important than the accuracy of the ref assemblies list. For example, generating project info to prepare auto-completion list for VSCode, Notepad++ or Sublime Text plugins. It is because auto-completion does not expect any delays (e.g. downloading the package). It should either succeed or fail but very quickly. The work around is that the user need to execute the script at least once and all the packages become available and auto-completion works fully.

  2. User defined NuGet.config is an excellent idea. And it can be isolated for the -vs use-case.
    Can I ask you to log it as an enhancement, please?

@husk3r husk3r deleted the nuget branch December 6, 2021 08:40
@husk3r
Copy link
Contributor Author

husk3r commented Dec 6, 2021

Sure. I'll start to play around getting the current valid NuGet.confg and create a new pull request when I've the feeling that it is worth to do so ;-)
The good thing about the NuGet.config copy approach is that Visual Studio itself downloads and resolves the missing nuget packages. Give me some time and let's see.

oleg-shilo added a commit that referenced this pull request Dec 7, 2021
  Triggered by PR #263: Use and forward additional nuget arguments
@oleg-shilo
Copy link
Owner

Agree.
I have added the code that I think will do what required. Check out branch "issue_263_nuget.config".

csscript.cs:
image
Will merge it if it does what you want it to to.

@husk3r
Copy link
Contributor Author

husk3r commented Dec 9, 2021

I had a chance to look at the code and did some investigations. First question is, where to copy the config files.
It should be a method which is executed in both scenarios, cscs -vs myscript.cs and cscs myscript.cs.

How about:

internal static string CreateProject(CompilerParameters options, string[] fileNames, string outDir = null)

And here I would add a method like that (untested):

        public void PrepareNuget(string scriptFile, string projectFile)
        {
            string nugetConfig = "NuGet.config";
            string packagesConfig = "packages.config";

            string scriptDirectorySource = scriptFile.GetDirName();
            string scriptDirectoryDestination = projectFile.GetDirName();

            string scriptPackageConfigSource = scriptDirectorySource.PathJoin(packagesConfig);
            string scriptPackageConfigDestination = scriptDirectoryDestination.PathJoin(packagesConfig;
            string scriptNugetConfigDestination = scriptDirectoryDestination.PathJoin(nugetConfig);

            if (File.Exists(scriptPackageConfigSource))
            {
                File.Copy(scriptPackageConfigSource, scriptPackageConfigDestination);
            }

            DirectoryInfo directoryInfo = new DirectoryInfo(scriptDirectorySource);
            while (directoryInfo != null)
            {
                string currentNuGetConfig = directoryInfo.FullName.PathJoin(nugetConfig);
                if (File.Exists(currentNuGetConfig))
                {
                    File.Copy(currentNuGetConfig, scriptNugetConfigDestination);
                    break;
                }
                // Look in parent directory
                directoryInfo = directoryInfo.Parent;
            }
        }

What do you think?

Edit: I forgot to mention that the copy must take place before calling

public string[] Resolve(string[] packages, bool suppressDownloading, string script)

But at that point the project / build directory is not there since it will be created in CreateProject (as far as I can see).

@oleg-shilo
Copy link
Owner

I agree with your approach. all good.
According to NuGet documentation, the config files should go to the solution/project folder as in the code I shared with you.

But I will need to check if the CreatePeroject call order needs to be thought through. I need to do some testing/investigation.

will keep you informed. X-mass break will give me some time for playing around :)

oleg-shilo added a commit that referenced this pull request Dec 29, 2021
@oleg-shilo
Copy link
Owner

After some further investigation I found that there are limit on what can be done with respect to consistency of "both scenarios, cscs -vs myscript.cs and cscs myscript.cs".

The reason for this is that cscs myscript.cs is not using dotnet.exe engine by default (it's extremely slow) but csc.exe. Meaning that cs-script cannot use neither packages.config nor NuGet.config as csc simply does not support packages. The same applies for another supported compiler Roslyn.

Interestingly enough Visual Studio is also moving away from the config files. Thus my experiments with VS2022 showed that even of the IDE setting are defaulting the use of packages.config a freshly created project will always use project package references instead and prompt user (on project opening) to get rid of packages.config if it is included in the project created with VS2019.

So this is what I have done for the next release of CS-Script (targeting .NET 6) that I am working on:

  • cscs -vs myscript.cs
    • both packages.config are NuGet.config fully supported
  • cscs myscript.cs
    • packages.config is supported by the script engine reading config file entries and treating them the same way as //css_nuget... directives.
    • NuGet.config is supported by by the script engine copying the config file to the dummy project file that is used to trigger downloading packages.

oleg-shilo added a commit that referenced this pull request Jan 10, 2022
- Added .NET6 support
- Issue #271: Any Special considerations running in a linux docker container?
- Added support for `packages.config` and `NuGet.config`
  Triggered by PR #263: Use and forward additional nuget arguments
- Various changes for .NET 6 porting
- Create NuGetCache directory on environments without an existing package directory
- Open main script in Visual Studio when project is loaded
- PR #265: Do not overwrite return code set in Environment.ExitCode
- Issue #264: Spelling issue?
- Issue #260: Double Entry Point Definition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants