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

Second pass on fixing 2883 #3838

Merged
merged 8 commits into from
Feb 6, 2022
Merged

Second pass on fixing 2883 #3838

merged 8 commits into from
Feb 6, 2022

Conversation

baronfel
Copy link
Contributor

The root problem there was being too brittle around the assumption that every reference that was in the references file for a project would be in the dependencies file as a direct dependency. If that didn't hold then we couldn't discover the version requirement for the reference, and we'd crash.

Now we offer a helpful message to the user suggesting that they add the package to their dependencies file for completeness.

In addition group handling has been much improved, so that we can support any package reference a user might reference from non-main groups in their paket.reference files.

The major TODO here is to incorporate the files I've checked in to the repo into a proper integration test, as well as adding a couple more once that initial mechanism is all set up.

@forki
Copy link
Member

forki commented Apr 29, 2020

Could you please check the tests? Thx

@baronfel
Copy link
Contributor Author

Oh absolutely I intend to check up the tests, I just pushed this as a draft to:

  • get feedback on the group-modeling changes,
  • get feedback on the logging/warn mechanism (I'd like to have a nice warning for the referenced-a-non-direct-dependency-directly use case,
  • signal that I was still working on the feature

@baronfel
Copy link
Contributor Author

Alright, now I've added integration tests for the fix-nuspecs changes.

When we encounter packages that are referenced in paket.references but not declared in paket.dependencies, we don't crash anymore (this was the reason for the revert), instead we issue a warning on the trace mechanism to the user.

How do you feel about the changes I made to the group manipulations in fix-packages? And do you have any things you'd like to see changed in the messages themselves? Did I use the correct mechanism for messaging?

@baronfel
Copy link
Contributor Author

Appveyor test failures seem be some kind of permissions error on the new directories for the tests I made?
Travis failures seem to be the same diffs error seen on other PRs recently, I'm not at all convinced it's to do with my changes here, since I've only touched the fix-nuspecs command, nothing to do with generating project file changes for old-sdk projects.

@forki forki closed this May 25, 2020
@forki forki reopened this May 25, 2020
@NicoVIII
Copy link
Contributor

Hi!

Sorry, if I look demanding or pushy. But I'm really interested in this feature, so I wanted to ask what has to be done here to finish it?

It sounds like the feature was ready, the PR is marked as draft and I guess it is not reviewed.
I personally don't know how the internals of paket work, but is there something I can do?

@baronfel
Copy link
Contributor Author

I think the last remaining stuff is adding the integration tests I mentioned in the initial comment + get a review. A review would be a lot easier/safer with integration tests IMO :)

@krauthaufen
Copy link
Contributor

krauthaufen commented Jan 10, 2022

any progress here?

We're currently trying to make our projects compile with the standard dotnet toolchain (without fake scripts/etc.) and integration with dotnet would therefore be pretty awesome.

I'd be happy to help here.

Also (since many F# projects use pretty much the same layout) it would be immensely helpful if paket had an option like --releasenotes bla.md for pack reading version/notes from a markdown file and something like --keyFile mynugetkey for publish

@forki
Copy link
Member

forki commented Jan 11, 2022

@baronfel anything I need to do?

@baronfel baronfel marked this pull request as ready for review January 11, 2022 13:23
@baronfel
Copy link
Contributor Author

Let me rebase this and see where CI is sitting. It looks ok to me, pending error message approval from you @forki and getting CI green.

@baronfel baronfel changed the base branch from bugfix to master January 11, 2022 13:25
@forki
Copy link
Member

forki commented Jan 18, 2022

now there are conflicts

@baronfel
Copy link
Contributor Author

rebased, thanks!

@forki
Copy link
Member

forki commented Feb 5, 2022

Unfortunately there are still couple of compile errors

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.

4 participants