Stream: t-cargo/PubGrub

Topic: PubGrub guide


view this post on Zulip Matthieu Pizenberg (Oct 30 2020 at 17:50):

I'm interested in feedback for the current state of the PubGrub guide, available here: https://5f9c4e71d7f5850c5e58c7b5--pubgrub-rs-guide.netlify.app/

Best is to give feedback directly on the github PR pubgrub#45

view this post on Zulip Alex Tokarev (Nov 15 2020 at 18:03):

Just started looking through the guide now that I'm finished with PR reviews.
Dropping link to the latest guide here for convenience: https://pubgrub-rs-guide.netlify.app/

Just wondering about Netlify: what advantages does it have compared to github pages? Do you use Free tier?

view this post on Zulip Alex Tokarev (Nov 15 2020 at 18:05):

Though if you version cooking recipes, well you are a very organized person.

:laughter_tears:

view this post on Zulip Matthieu Pizenberg (Nov 15 2020 at 18:06):

Yes netlify is free for static website. It is also provides https (github too) and is easy to configure with an external domain. For example https://matthieu.pizenberg.fr is hosted on netlify even though I pay the pizenberg.fr domain with OVH (french hosting service)

view this post on Zulip Alex Tokarev (Nov 15 2020 at 18:13):

Cool! I guess the choice of Netlify rather than Github Pages was due to your familiarity with it :smile:

view this post on Zulip Matthieu Pizenberg (Nov 15 2020 at 18:15):

It's also convenient to not have a build artefacts committed in a branch of the repository

view this post on Zulip Alex Tokarev (Nov 18 2020 at 14:20):

https://pubgrub-rs-guide.netlify.app/testing/benchmarking.html

The link to the benchmarking issue does not work. Maybe we can have TODO and 2 future sections commented out so that they are not present in the published version?

view this post on Zulip Alex Tokarev (Nov 18 2020 at 14:21):

https://pubgrub-rs-guide.netlify.app/version_solving.html#semantic-versioning

Should we note that we don't support a full spec in our provided type? Prerelease and build number

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 14:21):

Yeah actually I was thinking of clearing that section and link to the just announced benchmarking book

view this post on Zulip Alex Tokarev (Nov 18 2020 at 14:23):

Are you fine with me just dropping comments in this thread as I go along with the guide?

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 14:24):

yep

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 14:25):

Should we note that we don't support a full spec in our provided type? Prerelease and build number

Actually that initial section is very broad and does not mention our implementation of semantic versioning. But on the next page (https://pubgrub-rs-guide.netlify.app/pubgrub_crate/intro.html) it is mentionned, so I should add the note here.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 14:26):

Are you fine with me just dropping comments in this thread as I go along with the guide?

I'll make modifications on the fly then.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 14:28):

Matthieu Pizenberg said:

Should we note that we don't support a full spec in our provided type? Prerelease and build number

Actually that initial section is very broad and does not mention our implementation of semantic versioning. But on the next page (https://pubgrub-rs-guide.netlify.app/pubgrub_crate/intro.html) it is mentionned, so I should add the note here.

I've noticed we there is an information about it on the next page, maybe we should still explicitly mention omitting those 2.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 15:25):

https://pubgrub-rs-guide.netlify.app/pubgrub_crate/offline_dep_provider.html

It would be nice to have the example checked by Rustdoc. I wonder if we could add that example to the main repo (where it's tested) and crosslink it into the book? Not sure if mdbook supports such usage.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 15:27):

yeah, would be nice to have code checked but I'm not sure how that would be possible

view this post on Zulip Eh2406 (Nov 18 2020 at 15:28):

I think Eric has some way getting book code checked in the cargo repo, we could look there or ask him.
But I would not delay 0.2 for it.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 15:33):

My comments here are just general thoughts as I read the guide not connected with 0.2. We can create issues and fix them whenever.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 15:35):

https://pubgrub-rs-guide.netlify.app/pubgrub_crate/custom_dep_provider.html

I see @Matthieu Pizenberg has already updated the guide regarding choose_package_version, I thought it's still on my todo list :smile: Thank you!

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 15:36):

It would be nice to have the example checked by Rustdoc. I wonder if we could add that example to the main repo (where it's tested) and crosslink it into the book? Not sure if mdbook supports such usage.

I forgot already but in fact that example is already in pubgrub examples https://github.com/pubgrub-rs/pubgrub/blob/dev/examples/doc_interface.rs

view this post on Zulip Alex Tokarev (Nov 18 2020 at 15:36):

I forgot already but in fact that example is already in pubgrub examples

Right! The only issue is it getting out of sync then.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 15:36):

Yep, I had a lot to do at that time still so I asked you but now I could do it so it's done

view this post on Zulip Alex Tokarev (Nov 18 2020 at 15:39):

Damn, doing multiple things at once backfires. I've just paid for 2 quarters of medical insurance when the first one was already paid for in May.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 15:45):

Sorry for the offtopic :smile:

https://pubgrub-rs-guide.netlify.app/pubgrub_crate/custom_dep_provider.html

Finally, there is an optional should_cancel method.

That makes me remember about the change I wanted to make before 0.2! Or at least discuss with you. This method returns Result when a simple bool would suffice. We could change it to bool and still return the error we do now.

The only downside would be dropping user defined context from the error message, but is it a downside? Presumably users know why they want to cancel resolution. Unless they provide multiple ways to cancel with different errors... Is it a legit usecase?

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 15:49):

In my mind, this function is temporary. The result fits well with the output of the other methods and enables usage of ? for the implementer, which is nice. I very much think that user will come and say "it would be nice to have this, that and that" etc. So I don't think we should bother with it now. It will change for sure

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:18):

enables usage of ? for the implementer, which is nice.

In what situation does the user has to use it themselves? It enables ? for us, but IMO not worth it compared to using if in 1 place.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:30):

Even if it succeeds, we want to distinguish the cases where the answer is that we don't know the package dependencies and the cases where the package has no dependencies.

Maybe instead of "we don't know" we should emphasize that it's the user who can't fetch them for some reason, because "we" means library or its authors in the guide.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 16:32):

It enables ? for us

I meant if they do fallible stuff inside should_cancel, with it returning a result they can just use ?. Otherwise they would have to handle the potential errors in a more tedious way, a bit like the main function is anoying to deal with since you can't use ? in it. But I don't think that's super important anyway. There will be people asking for stuff regarding that function, so there is a very high probability of it becoming something else entirely. Let's just leave it as is for the time being and wait for feedback first.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:33):

https://pubgrub-rs-guide.netlify.app/pubgrub_crate/caching.html

A complete caching example based on the OfflineDependencyProvider is available in examples/caching_dependency_provider.rs.

We can add a link here

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:36):

By the way, I don't know if it's "complete" since there is no caching in the implementation of choose_package_version and the comment saying it's just for simplicity got removed. Not sure if this is an issue though.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 16:39):

By the way, I don't know if it's "complete"

"complete" here was more to express that it's actually runnable, not just the skeleton of the code. How would you say that?

view this post on Zulip Eh2406 (Nov 18 2020 at 16:42):

"a runnable example of this is in "

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:47):

Complete is fine, maybe I'm just worried I didn't implement caching of that part of the example

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:48):

https://pubgrub-rs-guide.netlify.app/pubgrub_crate/strategy.html

The documentation of Pub (PubGrub implementation for the dart programming language) states the following.

Pub chooses the latest matching version of the package with the fewest versions that match the outstanding constraint. This tends to find conflicts earlier if any exist, since these packages will run out of versions to try more quickly. But there's likely room for improvement in these heuristics.

That's what we say about it, not Pub :smile: Looking for an actual quote atm...

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 16:50):

That's what we say about it, not Pub

here you go :)
https://github.com/dart-lang/pub/blob/master/doc/solver.md#decision-making

view this post on Zulip Eh2406 (Nov 18 2020 at 16:50):

It is from https://github.com/dart-lang/pub/blob/master/doc/solver.md#decision-making

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:51):

It is an actual quote! My bad :sweat_smile:

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:53):

What when did matklad contributed to pubgrub?! I swear Alex is everywhere :grinning:

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:53):

I see him in contributor list of solver.md

view this post on Zulip Eh2406 (Nov 18 2020 at 16:54):

Hi fixed some documentation. about PubGrub

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 16:54):

typos and clarifications apparently

view this post on Zulip Alex Tokarev (Nov 18 2020 at 16:55):

That means maybe we could interest him in our implementation ^_^

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:02):

I want to add a link to that quote. Is there a better way to link than this?
https://github.com/dart-lang/pub/blame/master/doc/solver.md#L446-L449

It a) references master rather than particular revision and b) references blame rather than plain text.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:09):

Github has no problem linking to lines of source code, but when it's md file it uses a previewer without that functionality it seems like :frown:

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:10):

Found a way to link to a released version: https://github.com/dart-lang/pub/blame/SDK-2.10.0-64.0.dev/doc/solver.md#L446-L449
But it still links to blame. Is it fine?

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:16):

I guess so, could not find any better

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:19):

https://github.com/pubgrub-rs/guide/pull/1
Added with some wording changes.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:22):

looked good, merged it

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:22):

I'm doing small typo fixes without PRs, this was a larger change so submitted like that in case something was controversial.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:22):

Thank you!

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:32):

https://pubgrub-rs-guide.netlify.app/contributing.html

We are mentioning 1 feature required by cargo, maybe we can say trying to implement any one of them would be great. With a link to https://github.com/pubgrub-rs/advanced_dependency_providers/issues maybe.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:36):

Yes we can. The multi-version selection is something altering the core mechanism of pubgrub, it changes the rule "one version per package" so this requires more thinking and I wanted to put some light on that to focus people brain on that problem XD

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:37):

https://pubgrub-rs-guide.netlify.app/pubgrub_crate/solution.html

So instead of seeing things like this in the report:

Because there is no version of foo in 1.0.1 <= v < 2.0.0 and foo 1.0.0 depends on bar 2.0.0 <= v < 3.0.0, foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0 <= v < 3.0.0.

you may have directly:

foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0 <= v < 3.0.0.

We might want to refer to a specific example used for generating this message. Without context it's not clear where does foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0 <= v < 3.0.0. come from and why this is a problem.

Also without an example I can't understand the longer explanation :laughing:

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:38):

But we can definitely link to that repo issue and to issues in the main repo as well saying that anything in there, as usual is good to work on

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:38):

Yes we can. The multi-version selection is something altering the core mechanism of pubgrub, it changes the rule "one version per package" so this requires more thinking and I wanted to put some light on that to focus people brain on that problem XD

Right, let's throw enthusiasts at the hardest problem :grinning_face_with_smiling_eyes:

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:41):

We might want to refer to a specific example used for generating this message. Without context it's not clear where does foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0 <= v < 3.0.0. come from and why this is a problem

Oh I see. That actually isn't a problem (at least not a full problem) just an extract from the explanation of a problem. And the example is just to show the transformation.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:42):

I'm not sure I took that from an actual code result, or if I did I don't know anymore which one

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:44):

lemme check quickly if there is a failing example with foo 1 depending on bar

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:45):

Yep, examples/linear_error_reporting

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:46):

You can comment the collapse_no_versions() line to see the difference

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:49):

I can add an ellipsis

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:50):

Because there is no version of foo in 1.0.1 <= v < 2.0.0
and foo 1.0.0 depends on bar 2.0.0 <= v < 3.0.0,
foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0 <= v < 3.0.0.
...

becomes

Because foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0,
...

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:51):

is that better?

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:52):

Better! By the way, the line differs. In the guide:
foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0 <= v < 3.0.0.
Here:
Because foo 1.0.0 <= v < 2.0.0 depends on bar 2.0.0,

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:52):

Yep just saw that now too

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:52):

ok changing

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:53):

Oh I also parsed the full uncollapsed example now!

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:54):

Because there is no version of foo in 1.0.1 <= v < 2.0.0
This means Because no versions of foo exist in the range 1.0.1 <= v < 2.0.0

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:54):

Yes, was that unclear?

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:54):

well error reporting do need work XD

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:54):

I had trouble connecting this statement and the fact that's why only foo 1.0.0 was tried

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:55):

I just didn't understand what does that mean and why that sentence is there

view this post on Zulip Alex Tokarev (Nov 18 2020 at 17:57):

By the way, I just had another thought regarding bump method in Version trait. We needed that because we write 1.0.1 <= v < 2.0.0 here, right?
We can just say 1.0.0 < v < 2.0.0 instead! :smile:

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 17:59):

Nope that's not where its needed

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:00):

It is needed in another place in error reporting indeed. One place that transforms 1.0.0 <= v < 1.0.1 into 1.0.0

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:01):

But it's mostly needed in another place in the solver code, to convert versions into exact ranges. So doing the reverse that this example above in reporting

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:01):

Ah, I thought it was used here because it seemed like a bumped previous version. I see

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:01):

Yes, was that unclear?

I think I understood what confused me

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:02):

And in fact it could be avoided if we introduce another thing and complexify the range type but we can discuss that later

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:03):

When you see a sentence like:
Because there is no version of foo in 1.0.1 <= v < 2.0.0
you expect a second part like:
...that falls within range.

If you see something like:
Because there are no versions of foo in 1.0.1 <= v < 2.0.0
you don't, it's a complete sentence.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:04):

I guess I just understood a hidden mental reasoning for my previous NoVersions PR :laughter_tears:

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:04):

I guess I just understood a hidden mental reasoning for my previous NoVersions PR :laughing:

I guess we're at the root of the problem: language ahah

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:09):

Do you mind if I submit a change with this wording to reporting part? Pluralizing it seems like the smallest possible change. We can also choose to implement completely different wording and do that instead. I'm thinking that because when you said

well error reporting do need work XD

you may have had larger changes in mind that would also affect this.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:11):

oh there are definitely larger things to do ahah. We've not even touched error reporting yet. For the pluralization I'd wait I think. Reformulation in a different way would be better.

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:12):

https://pubgrub-rs-guide.netlify.app/testing/property.html#comparison-with-a-sat-solver

This might be worth putting into a separate section next to https://pubgrub-rs-guide.netlify.app/testing/property.html
I mean putting it into 4.2 and having benchmarking as 4.3.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:12):

Also I'm not sure it's clearer in the plural form, it may depend on your language learnings

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:13):

Alex Tokarev said:

https://pubgrub-rs-guide.netlify.app/testing/property.html#comparison-with-a-sat-solver

This might be worth putting into a separate section next to https://pubgrub-rs-guide.netlify.app/testing/property.html
I mean putting it into 4.2 and having benchmarking as 4.3.

can do

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:16):

done

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:17):

Also I'm not sure it's clearer in the plural form, it may depend on your language learnings

I feel like this is some kind of a rule but I can't link to it :distraught:
I used to take classes to prepare for C2 exam with a British guy, if he doesn't know than I completely made it up :laughter_tears: I'll ask him since I'm curious myself even without this argument.

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:21):

Damn I made a mistake than forced-push --amend but the amended commit does not trigger ci. Will wait for your next comment ^^

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:22):

You can launch CI manually from the Actions tab

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:23):

cool, oh actually it's there, queued

view this post on Zulip Alex Tokarev (Nov 18 2020 at 18:23):

Also it might have worked? The section disappeared from https://pubgrub-rs-guide.netlify.app/testing/property.html. It didn't appear as 4.2 though

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:24):

yep 1sec I just re-run it now

view this post on Zulip Matthieu Pizenberg (Nov 18 2020 at 18:24):

cool it's there :)


Last updated: Oct 21 2021 at 21:32 UTC