search

dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.73k stars 1.02k forks source link

.NET project file not updated #2218

Open pascalberger opened 6 years ago

pascalberger commented 6 years ago

While packages.config file was updated the csproj file was not updated in this PR: https://github.com/bbtsoftware/TfsUrlParser/pull/25/files

feelepxyz commented 5 years ago

@pascalberger hey sorry for the incredibly slow response on this! Is this still an issue for you? Looks like you've removed your packages file?

pascalberger commented 5 years ago

@feelepxyz The error still occurs from time to time on projects with packages.config

justalemon commented 5 years ago

I can also confirm this on justalemon/GGOV#16. Had to fix the csproj manually.

You can see how bad this is by taking a look at the respective AppVeyor build.

feelepxyz commented 5 years ago

Sorry, just realised this is a known issue and @greysteil already got back to you on it! To fix this issue is unfortunately a pretty major piece of work so won't get to any time soon :((

We're hopefully getting some resource to work on this in the next few months.

justalemon commented 5 years ago

Hi, there has been any progress on this?

cc @feelepxyz

greysteil commented 5 years ago

Sorry for the wait on this one - we're doing some infrastructure work that has taken up all of my time.

The change required here isn't trivial, as Phil says, so it's not something I can promise we'll get to immediately, but I'd really like to have it fixed. I'm hoping we'll have some .NET expertise on the team in the relatively near future, which will mean we can power through this and a couple of other issues 🤞.

caiofbpa commented 5 years ago

I'm having this issue too, and I don't understand why this isn't a trivial change.

In projects that have only the .csproj files, mostly .NET Standard projects, dependabot updates the version number accordingly.

In projects with package.config files, dependabot updates the version number too, it just needs to do the same on the .csproj too. I'm assuming the logic is already there, it just needs to be called appropriately.

justalemon commented 5 years ago

.csproj is just XML, so there is not a lot to do other than search for an ItemGroup that contains a Reference where Include starts with the package name(ish).

greysteil commented 5 years ago

Just digging into this now - looks like the issues described here are quite separate.

@justalemon - it looks like in justalemon/GGOV#16 Dependabot missed your ...csproj declaration because you use Reference, rather than PackageReference, as the tag name. I couldn't find any docs on that - could you point me to some? As you can see, that isn't included in Dependabot's selector.

@pascalberger - it looks like the issue on your repo is from a very old PR, and you've re-organised your project since. Do you have a more recent example of the issue on one of your repos?

@caiofbpa - can you link to a PR where you've seen this problem and I'll dig into that?

caiofbpa commented 5 years ago

@greysteil thanks for taking the time to look into this.

We only use dependabot in private projects, but assuming you can look into dependabot's logs for a specific PR, here it is: https://github.com/mercos/offline/pull/1965

Dependabot should have also edited the .csproj file replacing 1.3.4 to 2.1.0 in the line where Refractored.FloatingActionButton is present. Currently I have to do that manually, so I'm opening a different PR for that.

pascalberger commented 5 years ago

@greysteil Here's a newer one: https://github.com/cake-contrib/Cake.Issues.Reporting.Generic/pull/148. For this project and package everytime only packages.config is updated and csproj not.

justalemon commented 5 years ago

@greysteil This is not documented for some reason. All of my .NET Framework projects have Reference instead of PackageReference. The same applies for @pascalberger Cake.Issues.Reporting.Generic.

RohanNagar commented 5 years ago

I believe Reference is used for assembly references. In your case @justalemon, your packages.confg is the "PackageReference" equivalent, and the binaries are downloaded from the Nuget feed into your local machine. Then, in the csproj since you already have the assemblies on the machine, it uses Reference to reference the assembly directly.

In other words, using PackageReference in a csproj is a direct reference to a nuget package. But if you have a packages.config, that takes the place of the nuget package reference, so you need to use Reference to reference the assembly that was downloaded from the packages.config declaration.

I'm not sure why I can't find great documentation on this, but it is standard. I have a project where I directly use Reference to a local dll assembly.

Edit: Take a look at this (https://docs.microsoft.com/en-us/aspnet/web-forms/overview/deployment/web-deployment-in-the-enterprise/understanding-the-project-file#items-and-item-groups):

... the Reference list includes assemblies that must be in place for a successful build, the Compile list includes code files that must be compiled, and the Content list includes resources that must be copied unaltered.

greysteil commented 5 years ago

Got you. @caiofbpa, in your case it looks like it's a HintPath that's part of a reference that hasn't been updated, so all of these are connected.

I think Dependabot should be able to successfully update these references whilst continuing to use its current regex-based approach for .NET. We'll need new logic for finding and updating them as they're structured differently, but I think this is still just a straight find and replace for the version in the relevant blocks. Make sense?

caiofbpa commented 5 years ago

@greysteil Makes total sense, it should simply be a find and replace indeed.

Sometimes one dependency triggers updates on others, but that is an exception that only happens on old projects (which is my case) and shouldn't be a concern because with .NET Core this has changed and is no longer an issue. I can treat those cases manually here, the find and replace is enough for most cases.

caiofbpa commented 5 years ago

@greysteil Sorry to be possibly annoying, but do you have any news about this? It sounded like a quick and simple solution to a problem that is making dependabot quite unusable for some .NET projects.

adamralph commented 5 years ago

Would it be better to spend energy on switching to the new csproj rather than getting issues like this fixed?

caiofbpa commented 5 years ago

Everyone using Xamarin is stuck with Mono-like csproj for at least one project. It's not really my choice.

pascalberger commented 5 years ago

Would it be better to spend energy on switching to the new csproj rather than getting issues like this fixed?

I can't speak about what priorities Dependabot has, what resources are available and how much effort it is to fix this. But to say, for me this is not a critical issue. I can live with current restrictions for cases where I cannot switch to the new csproj format. Although there aren't many on my side and situation might be different for other people.

greysteil commented 5 years ago

Sorry for the slow progress on this one - it's not lack of willing. The GitHub acquisition announced on Thursday has been in the works for the last couple of months, so I haven't had as much time for issues like this as I would like.

I'm going to take a look at this now and see if I can get it done.

caiofbpa commented 5 years ago

@greysteil no problem at all! Congratulations on the acquisition, this is wonderful news for us, doubling the team means faster improvements and expanded support for other languages. Thanks for your solicitude and keep up the good work! If you need any more info, just ask. 😄 👏

ronaldbarendse commented 5 years ago

There's more at play here when using the old .csproj format with a packages.config file, besides updating the version in packages.config:

A lot has changed in the new csproj-format when using PackageReference, most of them to address these issues...

twalsh92 commented 5 years ago

Is there any update on this, as I'm currently hitting issues with dependabot not updating the project files?

greysteil commented 5 years ago

Not yet. If you link to some PRs that are incorrect from here it will help us build fixtures when we fix, though.

3F commented 5 years ago

Same here :(

DllExport project:

vsSolutionBuildEvent project:

justalemon commented 5 years ago

All of the PRs that have caused problems on my repos:

3F commented 5 years ago

Please note the following features with .NET projects:

new references records

Some updates may add new references in project files if we're talking about nuget manager.

For example, for projects below, the following direct reference are not needed. But NuGet dependency policy will add this anyway:

diff --git "a/NSBin/NSBin.csproj" "b/NSBin/NSBin.csproj"
index b46d601..8beb9c1 100644
--- "a/NSBin/NSBin.csproj"
+++ "b/NSBin/NSBin.csproj"
@@ -39,8 +39,17 @@
     <AssemblyOriginatorKeyFile>key.snk</AssemblyOriginatorKeyFile>
   </PropertyGroup>
   <ItemGroup>
-    <Reference Include="Mono.Cecil, Version=0.10.3.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e, processorArchitecture=MSIL">
-      <HintPath>..\packages\Mono.Cecil.0.10.3\lib\net40\Mono.Cecil.dll</HintPath>
+    <Reference Include="Mono.Cecil, Version=0.10.4.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e, processorArchitecture=MSIL">
+      <HintPath>..\packages\Mono.Cecil.0.10.4\lib\net40\Mono.Cecil.dll</HintPath>
+    </Reference>
+    <Reference Include="Mono.Cecil.Mdb, Version=0.10.4.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e, processorArchitecture=MSIL">
+      <HintPath>..\packages\Mono.Cecil.0.10.4\lib\net40\Mono.Cecil.Mdb.dll</HintPath>
+    </Reference>
+    <Reference Include="Mono.Cecil.Pdb, Version=0.10.4.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e, processorArchitecture=MSIL">
+      <HintPath>..\packages\Mono.Cecil.0.10.4\lib\net40\Mono.Cecil.Pdb.dll</HintPath>
+    </Reference>
+    <Reference Include="Mono.Cecil.Rocks, Version=0.10.4.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e, processorArchitecture=MSIL">
+      <HintPath>..\packages\Mono.Cecil.0.10.4\lib\net40\Mono.Cecil.Rocks.dll</HintPath>
     </Reference>
     <Reference Include="System" />
     <Reference Include="System.Core" />

....


### Choose/When

Moreover, some of this may use `Choose/When`: https://github.com/3F/vsSolutionBuildEvent/blob/c68aea2794ec70fde43ddbe77fd9a185fa6429f0/vsSolutionBuildEvent/vsSolutionBuildEvent.csproj#L165-L170

```xml
  <Choose>
    <!-- SDK15+ -->
    <When Condition="$(DefineConstants.Contains('VSSDK_15_AND_NEW'))">
      <ItemGroup>
        <Reference Include="Microsoft.Build.Framework, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Build.Framework.15.9.20\lib\net46\Microsoft.Build.Framework.dll</HintPath>

Even local updating via nuget works incorrect at all (see vsSolutionBuildEvent). So I don't know about dependabot.

Analyzer

Also please don't forget about Analyzer records:

https://github.com/3F/vsSolutionBuildEvent/blob/c68aea2794ec70fde43ddbe77fd9a185fa6429f0/vsSolutionBuildEvent/vsSolutionBuildEvent.csproj#L824-L829

    <When Condition="$(DefineConstants.Contains('VSSDK_15_AND_NEW'))">
      <ItemGroup>
        <Analyzer Include="..\packages\Microsoft.VisualStudio.SDK.Analyzers.15.7.4\analyzers\cs\Microsoft.VisualStudio.SDK.Analyzers.dll" />
        <Analyzer Include="..\packages\Microsoft.VisualStudio.Threading.Analyzers.15.8.209\analyzers\cs\Microsoft.VisualStudio.Threading.Analyzers.CodeFixes.dll" />
        <Analyzer Include="..\packages\Microsoft.VisualStudio.Threading.Analyzers.15.8.209\analyzers\cs\Microsoft.VisualStudio.Threading.Analyzers.dll" />
</ItemGroup>

Hope this can help isolate the problem. Thanks!

3F commented 5 years ago

I don't know... for .NET packages dependabot adds more inconvenience than improvements because I need to fix manually each PR for more than 40+ packages in each .NET based projects.

just again, today: https://github.com/3F/vsSolutionBuildEvent/pull/58 https://github.com/3F/vsSolutionBuildEvent/pull/57

Seems like I will disable .NET feature because it's hard :( Hope this will be fixed someday.

By the way, I also don't like control via .dependabot/config.yml (maybe because of this issue) but today this is the only way to add some ignored_updates. Or I just can't find something in your online app.dependabot.com

greysteil commented 5 years ago

Sorry to hear that but I would definitely disable in your case. Sadly we just don't have the resource to make the improvements that we'd like to at the moment - we're very happy to accept pull requests for them.

3F commented 5 years ago

I see. Same story for me. This is why this project looks great to save a bit of time. Thanks for this!

I have no time today but I'll try to consider later some PR to dependabot if it will be still an unresolved problem.

caiofbpa commented 5 years ago

@3F I use dependabot as a reminder of which libraries have new versions, and then I go and manually update them together. Dependabot closes the dependency PRs as soon as it detects that I updated manually. It's been very useful to me that way, I don't have to keep checking for updates manually on all projects.

3F commented 5 years ago

@caiofbpa

I use dependabot as a reminder

For some cases it must be a good idea. However, some projects may contain many references that's in ~regular updates~ ie. cyclical update at the same time* together with. This simply adds more unrelated tasks for your timeline.

then I go and manually update them together

The problem is that some dependencies such as VSSDK may expose too much "unrelated" opened PR. I can restrict it to 1 of course but it's more like I still need to check it and fix it manually for all others.

For example, yes, just one commit closes lot of dependabot's PRs:

https://github.com/3F/vsSolutionBuildEvent/commit/e50775c65409afeac5a2b934a8fa6a67b0830b1d

but ...

caiofbpa commented 4 years ago

stalebot just reminded me about this.

Just chiming in to let you all know that I don't need this anymore. We managed to convert our .csproj from using package.config to using PackageReference (more info here). This made dependabot work like a charm for all our .NET projects.

Thanks for the hard work!

bording commented 4 years ago

Given the complexity of properly updating a project that uses packages.config, I'd much prefer that you disable your arguably broken support for packages.config until you're able to get it working properly.

Right now Dependabot creates incorrect PRs for these projects, so they will never be mergeable. This just makes the PRs noise that I have to sort through.

jeffwidman commented 2 years ago

Re: the noise... for those who aren't aware it is possible to disable security updates or version updates.

As far as fixing the root cause, I'm afraid we probably won't get to this one for a while. If anyone has interest and wants to take a crack at it, we'd happily provide suggestions on where to start and review a PR.

marco-carvalho commented 1 year ago

really miss this feature 😢