allow additional files to be included in the nupkg #2561

analogrelay · 2015-08-25T22:30:31Z

fixes #848

blocked on aspnet/FileSystem#120 ... still waiting for a squirrel there ;). Anyone on this review should feel free to go review that and give me a squirrel there :P

With this change, the project.json can have a new packageFiles section, for example:

{
    "packageFiles": {
        "destination1/": "source1/**",
        "destination2/": "source2/**",
        "destination2/some_additional_file.txt": "source2a/somefile.txt",
        "destination3/": ["source3/a.txt", "source3/b.txt", "source4/c/**/*.ps1"]
    }
}

The value is a JSON object. Keys in this object (destination1, etc. in the above example) are destinations relative to the package root. P. If they end with a /, they are treated as directories, otherwise they are files. The associated value is one or more globbing patterns. If the globs end up pulling in multiple files, the destination on the left must be a directory, otherwise an error is reported (see tests for example). If the destination is a directory, the file name for each file is calculated using the stem of the globbing match (see aspnet/FileSystem#120).

Open Questions:

Name of PackageFilesSource class is weird, but I want to keep the Line/Column info around for error reporting...
Handling the "arity" of the source/destination pairs is still a little icky. Requiring that directories end with "/" seemed like the easiest because it allows us to have a clear error. Thoughts? That part can be tweaked both now, and after receiving user feedback in the remaining beta.

/cc @davidfowl @Tratcher @BrennanConroy @halter73 @lodejard

moozzyk · 2015-08-25T23:31:46Z

src/Microsoft.Dnx.Tooling/Building/BuildManager.cs

+                        yield return new PhysicalPackageFile()
+                        {
+                            SourcePath = file.Path.Replace('/', Path.DirectorySeparatorChar),
+                            TargetPath = Path.Combine(dir, file.Stem.Replace('/', Path.DirectorySeparatorChar))


I think it might break in case of absolute, incorrect paths - two cases that comes to my mind are:

/Windows/ -->You will probably end up witch \Windows\file.txt path
// --> you will end up with \

The source paths come from globbing which has already processed them and made them absolute. And the target paths are used relative to the ZIP file being generated so the worst they can do is mess up a user's package. In either case, if the user types an incorrect path, they will get an incorrect result, which doesn't seem bad to me :)

I think file.Stem is always relative so you are right - we should be always relative to the ZIP file and then it is on the user to make sure that targets are correct.

davidfowl · 2015-08-26T07:11:56Z

My eyes aren't used to destinations being on the left, it feels still feels odd.

davidfowl · 2015-08-26T07:13:22Z

src/Microsoft.Dnx.Runtime/PackageFilesSource.cs

+
+        public PackageFilesSource(IEnumerable<string> values, int line, int column)
+        {
+            Values = values.ToList().AsReadOnly();


Do all callers call an IList directly? If so change the input to list or array or something specific. This isn't a generic type that we want to expose to people is it?

analogrelay · 2015-08-26T16:25:53Z

I suppose we could do destination on the right. It's a little weird to have globbing patterns as keys in a dictionary, but it's not awful.

ChengTian · 2015-08-26T21:19:02Z

src/Microsoft.Dnx.Tooling/Building/BuildManager.cs

-                            packageBuilder.Save(fs);
-                            _buildOptions.Reports.Quiet.WriteLine("{0} -> {1}", _currentProject.Name, Path.GetFullPath(nupkg));
-                        }
+                        bool packageSuccess = packDiagnostics.All(d => d.Severity != DiagnosticMessageSeverity.Error);


!packDiagnostics.HasErrors()

ChengTian · 2015-08-26T21:37:57Z

With the following configuration:

{
    "packageFiles": {
        "tools/": "A.txt",
        "./tools/": "B.txt",
        "arbitrary/../tools": "C.txt"
    }
}

The current implementation will copy all three .txt files into one tools folder, which looks fine.

However, with

{
    "packageFiles": {
        "tools/final.txt": "A.txt",
        "./tools/final.txt": "B.txt",
        "arbitrary/../tools/final.txt": "C.txt"
    }
}

The behavior is undefined because we are using Dictionary, which is unsorted. So we don't know which .txt file gets copied as the last one and overwrites all previous ones.

moozzyk · 2015-08-26T23:09:52Z

Should we even allow overwriting files by default? feels scary (--force option would be useful)

analogrelay · 2015-08-27T17:20:58Z

I'd be ok with blocking files with duplicate final target paths.

analogrelay · 2015-08-27T17:21:17Z

I'd also be OK with banning path-traversal characters in the target paths since they are unnecessary :)

davidfowl · 2015-08-28T08:45:39Z

src/Microsoft.Dnx.Runtime/Project.cs

@@ -233,12 +236,25 @@ internal static Project GetProjectFromStream(Stream stream, string projectName,

            project.Dependencies = new List<LibraryDependency>();

+            // Files to be packed along with the project
+            var packInclude = rawProject.ValueAsJsonObject(PackIncludePropertyName);


Parse all of this lazily

I'm doing so, but it means more of the JSON is retained in memory without being parsed down to smaller data structures. I'm not sure that's actually useful. Perhaps with the ProjectReader change we could make the amount of data loaded configurable and then only have dnu pack load this much but that seems like a larger change.

Anyway, I'll lazy it for now and we can discuss a better option later.

Perhaps with the ProjectReader change we could make the amount of data loaded configurable and then only have dnu pack load this much but that seems like a larger change.

I'll look into that. I was thinking of a runtime project but this might be a better option. Either that or make it all lazy.

So a separate project class used by the runtime? That's not a bad idea either. Also, we should extract ProjectReader out into a package at some point (possibly just a shared source package). We'd still embed it in the runtime, but as @rynowak said it's useful in other situations.

analogrelay · 2015-08-28T21:48:31Z

⌚ @davidfowl @moozzyk

analogrelay · 2015-08-28T21:48:53Z

Travis failures are unrelated

davidfowl · 2015-08-28T21:50:48Z

src/Microsoft.Dnx.Tooling/Building/BuildManager.cs

+                                _buildOptions.Reports.Quiet.WriteLine("{0} -> {1}", _currentProject.Name, Path.GetFullPath(nupkg));
+                            }
+
+                            if (symbolPackageBuilder.Files.Any())


This is a new change

What? No it isn't, it was in my original PR.

The inline diff looks weird, split diff is clearer. Regardless, this was in the PR from early on.

analogrelay · 2015-08-28T22:20:41Z

⌚ @pranavkm @davidfowl

pranavkm · 2015-08-28T22:25:01Z

test/Microsoft.Dnx.Tooling.FunctionalTests/DnuPackAdditionalFilesTests.cs

+                        entries = archive.Entries.Select(e => e.FullName).Where(IsNotOpcMetadata).ToArray();
+                    }
+
+                    Assert.Equal(0, exitCode);


Could you print the stdError if the exitCode is non-zero?

It's already printed by the test.

pranavkm · 2015-08-28T22:27:34Z

fixes #848

moozzyk · 2015-08-31T17:11:32Z

makefile.shade

@@ -64,6 +64,7 @@ var PROGRAM_FILES_X86 = '${Environment.GetFolderPath(Environment.SpecialFolder.P
 var MSBUILD = '${Path.Combine(PROGRAM_FILES_X86, "MSBuild", "14.0", "Bin", "MSBuild.exe")}'
 var VS_REDIST_ROOT = '${Path.Combine(PROGRAM_FILES_X86, @"Microsoft Visual Studio 14.0\VC\redist")}'
 var WIN10_SDK_LIB = '${Path.Combine(PROGRAM_FILES_X86, @"Windows Kits\10\Lib")}'
+var VS_ONECORE_ROOT = '${Path.Combine(PROGRAM_FILES_X86, @"Microsoft Visual Studio 14.0\VC\lib\onecore")}'


@anurse - why was this needed?

I was in a situation where I did have the Windows SDK installed but still couldn't build for OneCore because I was missing files in this folder. I'll sync up with you in person.

andreasandreou · 2016-02-02T10:08:09Z

any idea when this will be available?

analogrelay · 2016-02-02T17:33:56Z

It should be available already, though the name was changed to packInclude.

andreasandreou · 2016-02-03T07:31:30Z

@anurse thank you, indeed you can include content using packInclude but there is still no way to get to import the content from package into the project which makes this pretty much useless.

analogrelay · 2016-02-03T17:32:55Z

Correct, that's something tracked over in the dotnet/cli repo, since DNX is being retired: https://github.com/dotnet/cli/issues/66

andreasandreou · 2016-02-04T07:02:35Z

thank you very much for the follow up @anurse, this is very good news, will track referenced issue.

dnfclas added the cla-already-signed label Aug 25, 2015

moozzyk reviewed Aug 25, 2015
View reviewed changes

davidfowl reviewed Aug 26, 2015
View reviewed changes

ChengTian reviewed Aug 26, 2015
View reviewed changes

davidfowl reviewed Aug 28, 2015
View reviewed changes

danroth27 mentioned this pull request Aug 28, 2015

Create a New NuGet Package with DNX dotnet/AspNetCore.Docs#52

Closed

analogrelay force-pushed the anurse/848-additional-files branch from 40952ae to a1229f3 Compare August 28, 2015 21:28

davidfowl reviewed Aug 28, 2015
View reviewed changes

analogrelay force-pushed the anurse/848-additional-files branch from 44fdd09 to 0e99ed1 Compare August 28, 2015 22:20

pranavkm reviewed Aug 28, 2015
View reviewed changes

analogrelay force-pushed the anurse/848-additional-files branch from 0e99ed1 to 8d67c50 Compare August 28, 2015 22:52

analogrelay added 2 commits August 28, 2015 17:20

allow additional files to be included in the nupkg

bb07b16

fixes #848

fix unrelated test

9e7a621

analogrelay force-pushed the anurse/848-additional-files branch from 710d9c1 to 9e7a621 Compare August 29, 2015 00:20

analogrelay merged commit 9e7a621 into dev Aug 29, 2015

moozzyk reviewed Aug 31, 2015
View reviewed changes

analogrelay deleted the anurse/848-additional-files branch September 22, 2015 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow additional files to be included in the nupkg #2561

allow additional files to be included in the nupkg #2561

analogrelay commented Aug 25, 2015

moozzyk Aug 25, 2015

analogrelay Aug 26, 2015

moozzyk Aug 26, 2015

davidfowl commented Aug 26, 2015

davidfowl Aug 26, 2015

analogrelay commented Aug 26, 2015

ChengTian Aug 26, 2015

ChengTian commented Aug 26, 2015

moozzyk commented Aug 26, 2015

analogrelay commented Aug 27, 2015

analogrelay commented Aug 27, 2015

davidfowl Aug 28, 2015

analogrelay Aug 28, 2015

davidfowl Aug 28, 2015

analogrelay Aug 28, 2015

analogrelay commented Aug 28, 2015

analogrelay commented Aug 28, 2015

davidfowl Aug 28, 2015

analogrelay Aug 28, 2015

analogrelay Aug 28, 2015

analogrelay commented Aug 28, 2015

pranavkm Aug 28, 2015

analogrelay Aug 28, 2015

pranavkm commented Aug 28, 2015

moozzyk Aug 31, 2015

analogrelay Aug 31, 2015

andreasandreou commented Feb 2, 2016

analogrelay commented Feb 2, 2016

andreasandreou commented Feb 3, 2016

analogrelay commented Feb 3, 2016

andreasandreou commented Feb 4, 2016

allow additional files to be included in the nupkg #2561

allow additional files to be included in the nupkg #2561

Conversation

analogrelay commented Aug 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Aug 26, 2015

Choose a reason for hiding this comment

analogrelay commented Aug 26, 2015

Choose a reason for hiding this comment

ChengTian commented Aug 26, 2015

moozzyk commented Aug 26, 2015

analogrelay commented Aug 27, 2015

analogrelay commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay commented Aug 28, 2015

analogrelay commented Aug 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay commented Aug 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Aug 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasandreou commented Feb 2, 2016

analogrelay commented Feb 2, 2016

andreasandreou commented Feb 3, 2016

analogrelay commented Feb 3, 2016

andreasandreou commented Feb 4, 2016