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

Package content is not restored correctly when installing a package from a nuget v3.3+ feed with the argument -NoCache when the package contains *.nupkg files #2354

Closed
simonejsing opened this Issue Mar 17, 2016 · 16 comments

Comments

Projects
None yet
5 participants
@simonejsing

simonejsing commented Mar 17, 2016

I have a Nuget package which itself contains other Nuget packages (*.nupkg files). When I install this package, the contained *.nupkg files are missing from the output location.

The issue only occurs when installing packages from nuget v3.3+ feeds. If I install the package from a local file share or a nuget v2 feed it works as expected and the output location has the contained *.nupkg files in it.

When I install a package from a nuget v3.3+ feed it is first copied and extracted to a machine cache which normally is under %appprofile%, but can be changed with the environment variable NUGET_PACKAGES. In this temporary location the package content is correct (it contains the *.nupkg files). It is when the package content is copied from the temporary location to the output directory that contained *.nupkg files end up missing.

Furthermore, the broken behavior is influenced by the use of the –NoCache switch to nuget.exe. If I don’t specify –NoCache, then the package content correctly contains all the .nupkg files.

To repro create a Nuget package that contains *.nupkg files, upload it to a nuget v3.3+ feed and restore it from there using the -NoCache argument to nuget.exe.

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Mar 23, 2016

For prioritizing purposes: Can you explain the scenario?

yishaigalatzer commented Mar 23, 2016

For prioritizing purposes: Can you explain the scenario?

@simonejsing

This comment has been minimized.

Show comment
Hide comment
@simonejsing

simonejsing Mar 23, 2016

We are a Microsoft team that runs the Engineering Systems for development of Microsoft Dynamics AX. The team is large, and we rely on Nuget to pull down packages for build dependencies in our internal Engineering Systems (based on CoreXt and VSTS if that is familiar to you). One of these packages happens to contain .nupkg files. When it is installed the content is incorrectly missing these .nupkg files causing build errors in our Continuous Integration system. This affects our CI builds as well as local developer builds.

As a temporary workaround we have switched to consume packages from the v2 Nuget endpoints which does not show this bug.

simonejsing commented Mar 23, 2016

We are a Microsoft team that runs the Engineering Systems for development of Microsoft Dynamics AX. The team is large, and we rely on Nuget to pull down packages for build dependencies in our internal Engineering Systems (based on CoreXt and VSTS if that is familiar to you). One of these packages happens to contain .nupkg files. When it is installed the content is incorrectly missing these .nupkg files causing build errors in our Continuous Integration system. This affects our CI builds as well as local developer builds.

As a temporary workaround we have switched to consume packages from the v2 Nuget endpoints which does not show this bug.

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Mar 23, 2016

That sounds like a design gone wrong, packages should be packages and not shipped inside other packages. I would advise changing your design.

In the meantime we figure out how to prioritize he fix here, you can also contribute it if you'd like to unblock yourself quickly (though ID still recommend not relying on nupkg inside nupkg)

yishaigalatzer commented Mar 23, 2016

That sounds like a design gone wrong, packages should be packages and not shipped inside other packages. I would advise changing your design.

In the meantime we figure out how to prioritize he fix here, you can also contribute it if you'd like to unblock yourself quickly (though ID still recommend not relying on nupkg inside nupkg)

@simonejsing

This comment has been minimized.

Show comment
Hide comment
@simonejsing

simonejsing Mar 23, 2016

Well I would tend to agree with you on a design gone wrong, but sometime you inherit something that you just have to live with.

That being said, I would strongly expect Nuget to treat the content of any package the same way, be it .nuspec, .nupkg, or any other file extension for that matter.

Btw. I also tested this with Nuget 3.4 and the bug still exists.

simonejsing commented Mar 23, 2016

Well I would tend to agree with you on a design gone wrong, but sometime you inherit something that you just have to live with.

That being said, I would strongly expect Nuget to treat the content of any package the same way, be it .nuspec, .nupkg, or any other file extension for that matter.

Btw. I also tested this with Nuget 3.4 and the bug still exists.

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Mar 23, 2016

Feel free to contact me offline.

yishaigalatzer commented Mar 23, 2016

Feel free to contact me offline.

@plequere

This comment has been minimized.

Show comment
Hide comment
@plequere

plequere Apr 5, 2016

We (VSEng) are also hitting this issue and had to downgrade our feed to v2. While I agree that the package isn't well formed, unfortunately we have no direct control over our packages since we're consuming packages from various sources in the VS build environment.

plequere commented Apr 5, 2016

We (VSEng) are also hitting this issue and had to downgrade our feed to v2. While I agree that the package isn't well formed, unfortunately we have no direct control over our packages since we're consuming packages from various sources in the VS build environment.

@yishaigalatzer yishaigalatzer added this to the 3.5 RC milestone Apr 5, 2016

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Apr 5, 2016

I suggest pushing back on the package owners to remove the nupkg's from the package, but we will take a look at a fix for 3.5. If you'd like to speed it up, feel free to jump in and suggest a pull request.

yishaigalatzer commented Apr 5, 2016

I suggest pushing back on the package owners to remove the nupkg's from the package, but we will take a look at a fix for 3.5. If you'd like to speed it up, feel free to jump in and suggest a pull request.

@simonejsing

This comment has been minimized.

Show comment
Hide comment
@simonejsing

simonejsing Apr 6, 2016

The bug is in PackageFolderReader in the GetFiles() method:

        public override IEnumerable<string> GetFiles()
        {
            var searchFolder = new DirectoryInfo(_root.FullName);

            foreach (var file in searchFolder.GetFiles("*", SearchOption.AllDirectories).
                Where(p => !p.FullName.EndsWith(PackagingCoreConstants.NupkgExtension, StringComparison.OrdinalIgnoreCase)))
            {
                yield return GetRelativePath(_root, file);
            }

            yield break;
        }

which excludes all .nupkg files

simonejsing commented Apr 6, 2016

The bug is in PackageFolderReader in the GetFiles() method:

        public override IEnumerable<string> GetFiles()
        {
            var searchFolder = new DirectoryInfo(_root.FullName);

            foreach (var file in searchFolder.GetFiles("*", SearchOption.AllDirectories).
                Where(p => !p.FullName.EndsWith(PackagingCoreConstants.NupkgExtension, StringComparison.OrdinalIgnoreCase)))
            {
                yield return GetRelativePath(_root, file);
            }

            yield break;
        }

which excludes all .nupkg files

@simonejsing

This comment has been minimized.

Show comment
Hide comment
@simonejsing

simonejsing Apr 6, 2016

will have a suggested fix out shortly

simonejsing commented Apr 6, 2016

will have a suggested fix out shortly

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Apr 6, 2016

Thanks for pointing out where the code filters out the nupkg files. It is obviously an intended behavior with a side effect for your scenario, I don't think that is where the bug/fix actually is. We don't want the nupkg to flow from the global package folder out to the target, and it is hard to tell what is the nupkg file exactly (because the filename might vary)

So a fix would be a lot more specific to your scenario, and will require careful examination of all the other scenarios.

yishaigalatzer commented Apr 6, 2016

Thanks for pointing out where the code filters out the nupkg files. It is obviously an intended behavior with a side effect for your scenario, I don't think that is where the bug/fix actually is. We don't want the nupkg to flow from the global package folder out to the target, and it is hard to tell what is the nupkg file exactly (because the filename might vary)

So a fix would be a lot more specific to your scenario, and will require careful examination of all the other scenarios.

@simonejsing

This comment has been minimized.

Show comment
Hide comment
@simonejsing

simonejsing Apr 6, 2016

my suggested fix is to exclude only .nupkg files that are in the root of the package

simonejsing commented Apr 6, 2016

my suggested fix is to exclude only .nupkg files that are in the root of the package

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Apr 6, 2016

That should work

yishaigalatzer commented Apr 6, 2016

That should work

@simonejsing

This comment has been minimized.

Show comment
Hide comment
@simonejsing

simonejsing Apr 6, 2016

The change that introduced the filter was related to this issue:
#1387

which talks about duplicate .nupkg files at the root of the package, so I don't think this should cause unintended side effects.

simonejsing commented Apr 6, 2016

The change that introduced the filter was related to this issue:
#1387

which talks about duplicate .nupkg files at the root of the package, so I don't think this should cause unintended side effects.

@simonejsing

This comment has been minimized.

Show comment
Hide comment
@simonejsing

simonejsing Apr 6, 2016

Which branch should I be requesting to be pulled into? I can't get the dev branch to build (some missing DotNet under msbuild folder), so I'm currently working in head detached mode from this tag: v3.4.0-rc-VSIX.

simonejsing commented Apr 6, 2016

Which branch should I be requesting to be pulled into? I can't get the dev branch to build (some missing DotNet under msbuild folder), so I'm currently working in head detached mode from this tag: v3.4.0-rc-VSIX.

simonejsing added a commit to simonejsing/NuGet.Client that referenced this issue Apr 6, 2016

zhili1208 added a commit to NuGet/NuGet.Client that referenced this issue Apr 21, 2016

@rrelyea

This comment has been minimized.

Show comment
Hide comment
@rrelyea

rrelyea Jun 7, 2016

Contributor

@zhili1208 , @simonejsing - sounds like this issue should be closed, right?

Contributor

rrelyea commented Jun 7, 2016

@zhili1208 , @simonejsing - sounds like this issue should be closed, right?

@rrelyea rrelyea added Type:Bug and removed Viewed labels Jun 7, 2016

@zhili1208

This comment has been minimized.

Show comment
Hide comment
@zhili1208

zhili1208 Jun 7, 2016

Collaborator

Yes, I merged this PR.

Collaborator

zhili1208 commented Jun 7, 2016

Yes, I merged this PR.

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