 Hey folks, this was entirely impromptu. I had some GitHub notifications I wanted to get to over the weekend and I figured if I'm gonna be doing open source programming anyway Why not stream it and if anyone thinks it might be interesting they can watch it and for people who don't care They can just not watch it and it's fine. Either way it cost me very little In this case I have like I think about two pages to get of notifications to get through I have no idea how far we're gonna get I'm not you know I'm not aiming to get through all of them today necessarily, but I figured you know There's a quite a variety of issues and PRs to look at here So I'll just you know sort of start from the back and move forward If you've watched the Previous one of these streams where I do open source development You'll see that you'll recognize some of these notifications as things that I skipped last time And I am also gonna skip them this time I think This one is probably the first one. I haven't really looked at in a while So this is a PR I Filed to rust itself To make it so that the AR 64 build of The rust tool chain builds LVM would support for compression It's a good question why did this get stuck I got stuck because we need Lib Z for AR 64 because I think it gets cross-compiled Don't think I want to pick up the context for that here. I think the fix here is actually pretty Simple but there's a lot to catch up on so I'm gonna mark that as unread and come back to it later The current starting experience is not user-friendly. This is They're totally right like the so this is for a crate called tracing timing tracing timing is a Subscriber for the tracing crate that So tracing lets you emit, you know events think of it as you can do logging except It's more structured and it doesn't have to just be logs It could be you know metrics any kind of observability event that you want for the execution of your program and then Tracing sort of separates the things that produce the events and the things that consume or subscribe to the events And in this case tracing timing is a subscriber to events that consumes events from tracing and in particular what it does is it looks at the Time between each event and then it keeps a histogram of where you end up spending most of your time So this can be useful for tracking down You know, let's say that you have a request handling function or something And it has a bunch of steps you can use is to figure out which steps are taking the longest across many requests It's a little fiddly to set up And you know that they quote from the docs here of the docs Basically saying you're sort of on your own here This is mostly a documentation problem, although Part of the complication here stems from the fact that it does concurrent recording and it's like if you have multiple requests that tries to Not, you know, do any synchronization between the requests because that would make Logging these events slow and instead it puts it on the consumer of the library to sort of say Now I want to gather the data from all the different threads that might have been gathering histogram data And that makes the API a little awkward, but it is also It also lets the Subscriber implementation be a lot more performant Documenting this is fairly tricky. It's also not a crate that I actively use anymore Not because it's not useful, but because I don't really write Software that needs this particular functionality These days right like I use this a lot for for Noria for trying to figure out, you know, and when a Particular, you know read or write requests to the database is being processed. Where does that time go and Subsequent that you have where can I shave it off? Where are the weird distributions where some part of the code is usually fast But sometimes has, you know Large peaks you can look for like bimodal distributions, for example in your histograms But these days because I work more on build tools that doesn't come up quite as much So here I'm gonna mark this as unread to use a lot of context to bring back in I don't think it would be very useful to see me Remember how this works and write up a bunch of docs So someone's asking in chat for you know, if they were to contribute to tracing timing, where would you start? I think this is a great place to start like I think that the documentation for tracing timing isn't awful as in You know, it does document how to use it what its output look like How to extract the histograms how it interacts with with the subscribers? So it's not as though this crate has no documentation It's more that the as as the issue says the the starting experience here is is kind of painful Because there are so many knobs that you sort of need to get right And part of this might be just putting in more examples to the crate for like this is how you set it up This is how you run it and I would be totally happy for someone to just like Contribute to those in fact, this is a crate that I would be happy to basically give away I I don't have a strong need to own it myself and you know in general giving away open-source projects is a little weird because I Don't want to just give it away wholesale to the the first person who comes along I want to sort of mentor them into it a little bit shepherd them into it and then you know And they've demonstrated that they have the the know-how to Responsibly take care of the crate going forward then you know, I would happily hand over the reins and Then the same is true for for timing. So I think if someone wanted to contribute to this You you start by making the you start by tackling a problem that is not you know about Fixing the low-level bugs in the code or anything like that But but more things like the getting starting experience or the API or the documentation like start there and then work your way into the the technical details And you know, I'm happy to take PRs on on any frontier really and you don't really need to You know focus on a particular thing to be able to get your PR in whatever PR You think would be useful to contribute I would be happy to take a look at and you know the open issues are always a good place to start All right mark is on red That brings us back to here So you'll notice some of my notifications here are On they're red, but they are not marked as done usually these are things where There's no more action necessarily from my side in terms of that issue or PR But there's some separate follow-up work I need to do so in the case of this crates index diff for example this is a bug in crates index diff that Not a bug let's call it a Missing configuration flag that I would like to get added and it's work that I should do at some point and contribute a PR and You know, there's no rush to do so It's just think of it as sort of a a very low priority to do list arguably I should just like bookmark it for later, but I never look at my bookmarks or This one when multi-line pasting in T-Mux new lines are stripped This is a bug that I occasionally run into on my work machine when SSHing to other machines and running Vim in T-Mux And it's this is more of a reminder or an easy way for me to find back to the issue if I find it again I want to share some more information about the context in which it happens But there's nothing for me to actively do about it Sharded counter optimization. Let's go look at this. So this is in flurry So flurry if you remember is our port of the Java concurrent hash map to rust The work here is one of the to-dos that we left when doing that porting was that The concurrent histogram has a Counter an atomic counter that's used to keep track of how many elements are in the map because you you don't want to count them Just like by walking the whole map because that's going to be really slow But if you keep a single atomic integer, that's also not great because all that means all Axises that update the map have to contend on this one integer value Some of them are going to increase it some of it decrease it and you know computers can be fairly efficient about doing those kind of Counter updates, but even so especially once you get to like, you know a multi socket architecture is like new monodes and such That doesn't really scale And so one of the optimizations is in the Java concurrent hash map as they use Effectively a sharded counter. So rather than having one counter that all the threats update You keep some larger number of counters and how you choose and and Which one you choose to update can vary and then you have readers read all of those counter and sum them The idea here being that writes no now only contend with other or updates only contend with other updates that access The same shard of the counter and readers just to read so so they they Parallelize really well. They don't need to take an exclusive lock on the underlying memory location And so this is someone who well so that there used to be this PR Which is looking to add shared counters and there was a lot of great work there And they posted a bunch of benchmarks and we see if I can find these Which actually it was a little unclear what the win was But in some cases you can see here for example So with this is with eight cores using the the shared counter versus just a single atomic counter You can see that it it ends up running faster or more operations per second than the equivalent atomic counter and if you have fewer threads It doesn't really give a speed up which also also makes sense And then ultimately Because they ended up moving away from the the implementation that was in this PR They opened a second PR and closed the first one That has these other up their operations instead and part of the thing I asked for in this PR was for the Logic to keep this sharded counter to be extracted into a separate crate And so they did that in the form of this fast counter crate, which is again using a basic sharded concurrent counter Now the question is where did I leave this off? Right, so this is one of the things that's that's complicated about being You know being a maintainer and asking for basically asking for another dependency is You don't have to audit that dependency right it in many cases not feasible to do a full review of all of your dependencies And practice like I just don't have the time but in this case this is you know pulling out a fairly performance critical part of Flurry the counter that is and I want to make sure that the underlying implementation. We're swapping it out with is actually a sensible one so in this case You know, I basically the PR here You'll you'll see if I look at the diff is actually fairly straightforward. So it takes a dependency on fast counter With some features and then it replaces the counter that we have in Flurry with a concurrent counter instead of The atomic ice ice we previously used It creates a new one where the number of shards is the number of CPUs is a pretty common way of sharding It doesn't have to be this way. You can also do things like shard by CPU socket or by You know by new Manot or something that makes the the operation for choosing which counter to update a little more complicated Then then if it's just a number of CPUs and you can just use the current CPUs ID as the the index into the list of shards for example But number of CPUs is fine here because this is one per map Generally, we're not gonna be very concerned about the memory use of you know Let's say you have a let's say you have 200 cores, right? Then this is 200 times the size of an integer That's how many counters you're gonna keep for one map It's not great, right? But also 200 courses a lot of course and the memory here is probably not really that bad It makes a note about this is probably for the this is for the length operation on the map It just adds a note, which is something I requested that the read you do might not ref might be You know in some sense different from the truth when you have a concurrent map They're gonna be updates continuously at the same time as you're calling length So what is the true length really if you read in the middle of multiple updates? And so really what this is saying is that In the absence of concurrent updates you get inaccurate results But concurrent updates that occur while the sum is being calculated might not be incorporated So basically this is sort of a an approximate read of the number of of items in the map and you see this just calls the the sum Method on the concurrent counter rather than loading that the single counter that we had before Yeah, so here I made the optimization that previously we had an optimization where So you can do multi updates in the map you can do things like Add multiple items remove and add an item or just replace an item in place and In those in the cases where the the length of the map doesn't actually change You don't need to do any atomic operation and this is the example here, right if we compare the The delta of the count to the previous count Sorry to the zero and if they're equal if it's equal to zero then we just load the counter We don't actually update it and this is an optimization that I said I wanted us to keep the other optimization I made here is that The count we want to make sure it takes an eye size rather than a new size Or rather Actually, that's not quite accurate. I want the counter That internally in the library that we're using to hold you sizes rather than eye sizes because counts are always positive But I wanted to be able to add eye sizes, right because if you have a Number that's a count you want to be able to subtract from that number and in fact That's what we did here right count used to be an atomic you size And so what that meant is if the eye size was negative We would have to call a different method with the positive value to subtract But this has the the benefit of you can technically store a larger count You know by a power of two because you don't need to store the sign Practices doesn't really matter. You're not going to get to a count. That's like two to the power of 64 anyway And we'll see so it looks like they actually made a change here. I haven't looked at what that was Removes on it is the unsafe okay, so you see the change is actually super simple here But that's because all of the complexity of that charting is being moved into this this fast counter crate So let's see what the updates were since last time You see I posted this August 20th. They got back to me August 21st, and it's now October 8th. I apologize Jack Thompson Right so I made some other observations that One is to remove the need for an unsafe cell. They kept a thread local to keep track of the current thread ID and They use an unsafe cell to be able to modify that thread local and instead you could use a cell which allows you to mutate through a shared reference as long as you're not Sharing the cell amongst multiple threads, which you don't because it's a thread local. So it's guaranteed to not be shared On nightly as I made the observation that on nightly you don't actually need to use this thread local at all because you can Just use the thread ID from the standard library, which has a as you 64 Just let you get the the identifier for the current thread anyway Sorry by a factor of two not a power of two you're entirely right And the other was why do you even need nightly here? Can't you just do this on stable and I think they use nightly because on nightly There's a you can do thread locals without pulling in an external dependency or rather. It's not even that It's you can do it without a macro. There's a macro in the standard library called thread local and On nightly you can use an attribute thread local instead just on a static variable That wind didn't really seem worth it to me if that's all we're doing in order to get Like if if it's really just a Syntactic thing it doesn't seem very worthwhile, but they're saying originally the performance difference was greater I guess we'll see how that came back September 7th been looking into this and a few thoughts and findings Thread ID as you 64 proved to be significantly slower. Oh Interesting Wonder why what does thread ID as you 64 do? It just reads self dot zero That's interesting. Oh, I'm guessing they call thread ID new Yeah, what they're probably doing is So there's a There's a thread current that gives you one of these thread objects and you can call You can call ID on it to get the thread ID and then you call as you 64 on it to get the the u64 I'm guessing that they're actually not storing the thread or Either that or dot ID is slow self dot inner Yeah, inner is an arc. So maybe they store the thread rather than the thread ID That's my guess because then you do have to go through an arc Which is a pointer in direction. It shouldn't really matter that much It's a pretty simple pointer chase That's interesting Two to three X lower I bet you showed the counter being slower than a regular atomic use size on every core count So a cell for a thread local that's real weird because a cell in rust is Just a wrapper transparent over an unsafe cell and When you call Set on a cell It's the same as a self dot replace And a self dot replace is just a memory place of an unsafe get so that something's real weird Like a cell is just an unsafe cell Interesting made it slower than a regular atomic use size That's wild Okay, so this is a good point this last one, which is Let me make this little bigger since he's here to read So this last one is interesting the observation here is that I think the way this is set up is that Every time you you modify the counter You actually instead of trying to figure out like which thread am I on or which CPU am I on and then operating the appropriate counter You pick randomly or actually maybe it uses a thread ID. I'm guessing given the code here, but imagine that when you When you remove an item, you're not necessarily on the same thread that added the item right so imagine Let's say that you know all the threads happen to choose the same counter every time So that one keeps incrementing and it's basically the value of that is the size of the map And then some thread comes along and tries to do a delete so it deletes a thing from the map and it started to decrement a counter But it decides to decrement a different counter than the one that everyone has happened to be using so far Which is fine because it's a sharded counter, but that other counter is zero So it can't decrement that counter and it doesn't know which counter has all the values and So if you make all of them eye sizes You don't have to deal with this problem because the sum across all of them is going to be fine anyway So you can just decrement to make it minus one and it's fine for one counter to be that That's interesting Okay Oh, sometimes the github quote thing is real weird. Let's try that again quote reply All right I'm always torn on whether to apologize for giving slow replies because on the one hand I Don't really have anything to apologize for all of this the volunteer work anyway But in the other like they did the did a thing and wrote up about a thing and I didn't get back to them for You know a month and that's not I don't feel good about that. So I'm torn But I don't know maybe I'm too nice, but I like it Let's see so here I think what I want to do is How about we say Looking at the stood source It seems like the arc is in the thread type Whereas the thread ID type is Just a plain integer, I'm guessing this ends up Being This ends up slow because you're either Calling thread current on every axis was just low or Or Keeping the thread around which means going through the arc each time to get to the numeric ID I Think the way to fix this is to Store the thread ID in us in a thread local That way you should have Super cheap access to it thread Is it thread ID or no lowercase d okay as you 64 is just a field access Without having to keep an ID counter yourself Let's see What do you think sell over unsafe sell Super weird the code for sell is really just an unsafe sell with Rapper Transparent and Sell set just does Mem replace unsafe zero should cost as much it cost exactly as much as Just setting the value and certainly not as much as Running atomic operations Hmm, could you give the raw numbers? You observed To see if we can figure out what's going on. Oh, yeah, so this is about removing nightly Nice It's enough there And a totally valid reason for using an eye size For the sharded counter, I think I'd still prefer for the API to return a U-Size Because the sum should never be negative Though maybe we Which I think we can simply assert with a debug Assert so it doesn't affect release performance release build performance It's fine to make ad take an eye size though, I think since It's a little weird, but not the end of the world Great. Oh, did I typo? I probably typoed where did I typo sharded? Did I typo sharded somewhere? Sharded I don't see my typo. Maybe I'm wrong. Let's see. What are people saying in chat? Not sure I like it passing in CPU count when creating the counter versus later use of ad which doesn't take in a thread ID Also, so I would think of number of shards is So the argument to new is the number of shards you want and it doesn't have to be the number of CPUs Just like if I think if I remember correctly the internals of the library is It uses the thread ID Modulo the number of shards to figure out which Shard to update and so I don't think it makes sense for ad to You know take an argument although, you know, there's a decent argument for Shared counter since they shared counter anywhere. It's no short counter It it might make sense to have a you know an additional mechanism For saying I want to do this ad on this particular counter But I think that's rare to be used in practice And what kind of project you need such a library it's super interesting But I work in web so I can't imagine what to use it for Actually in web development it comes up quite a lot not necessarily needing a shared counter But you do need a shared map quite often right to imagine things like Imagine you want to keep a cache in your so you have your little web server running And you know, there are all of your different Request handlers get to run in parallel and let's say that they want to cache some some stuff in memory to make it faster to access Well, now you want to share state using a key in memory between threats Which is what a concurrent hash map is for and If you are going to be doing that then you want that shared hash map to be as fast as possible and work as well in parallel as possible and If ultimately your shared map works really well in parallel except for the counter which ends up being centralized and contended on Then that's ultimately it's gonna end up being a sort of performance bottom like especially as you start scaling to a very large Number of cores and so that's why you care about this So you as a you know individual web developer might not use the sharded counter library directly But you're decently likely to use something like a Like a concurrent hash map or use some like let's say web framework that provides caching for you By using a concurrent hash map internally, right? The alternative would be you keep like a mutex over a regular hash map But that means that every request handler you have they're all gonna serialize on access to the cache Which you'd probably rather avoid Is there actually a case where a sharded counter would be a negative number It's a good question for a sharded counter The question is more about the algorithm that runs the sharded counter Could it ever because of all the concurrent axes? Could it ever be that a? Decrement happens before the increment that it's supposed to have incremented and in general that shouldn't be true Right because At least the aid well for counters actually it's weird because counters are usually updated lazily because they're not they don't need to be Perfect and so usually you know the concurrent algorithm you're using Tends to have these mechanisms that ensure that you can never remove something until it's been added right that shouldn't be possible There has to be some kind of you know in an app It's often by key where access to a given key is going to be sequential So you can't have it be removed before it's inserted But for the counter operations, maybe In which case maybe there's an argument for it's possible for for a brief period of time a counter to actually be negative, right? So imagine that Imagine you initially you started with an empty map One thread doesn't insert Another thread does a read and a remove and The two race so the insert code runs and it runs all the way up to where it increments the counter and If you assume that the counter increment happens at the very last outside of any other synchronization Then the insert hasn't returned yet But the value is still Observable in the map and then a read runs it finds the thing that was inserted and a remove runs to remove that value and the remove Runs to completion before this thread gets to continue so that includes the decrement And so there's a brief period of time after that remove finishes But before the insert finishes where the total set of the counter is actually negative That's a good point I think you're entirely right second thought in a concurrent setting with lazily updated counters, it's entirely possible that a counter ends that the sum of the counters ends up being negative for some period shorter period of time So I think we should Actually leave the return value as an eye size Someone's asking why thread ID modulo CPU count. It's not modulo CPU count. It's modulo the number of shards The idea being that you want in general you want different threads stack access different shards But you might have many more threads than you have CPUs or certainly then you have shards In fact for for something like this It's not obvious that you should choose number of CPUs as the number of shards as I said One example is you could choose the number of pneumo core pneumo sockets for example, or you could just choose like three Right just three is gonna cut your contention by a third Assuming the distribution of the thread IDs is roughly uniform in terms of their access and like You know, there's a big question here of what is the right number to choose? And it partially depends on your algorithm for choosing which shard to use on access You don't have to use thread ID you can pick randomly Although generating a random number is also takes a bunch of cycles, right? You want Because reads might have to access this but even for writes you want Generating the index to be relatively cheap as well and certainly not require any synchronization Which is why like the thread ID if you have it in a thread local anyway Is a pretty cheap way to do it another is choose randomly although then you have to generate a random number But you might get more uniform access over time Another is use the CPU ID modulo the number of shards you have Right and if the number of shards is the number of CPUs you're guaranteed to not contend with other CPUs But getting the CPU ID is actually fairly costly Not to mention you might be rescheduled between when you check your CPU ID and when you access the shard So it's not perfect that either Even just because of things like interrupts. So it's actually a fairly tricky game that you're trying to play here But but you can choose anything like you can choose, you know You're gonna come in an asynchronous concurrent execution context You might choose like the task ID or something that works fine, too It's just you want something that is Fairly likely to give somewhat uniform of an access pattern or distribution across shards rather Okay, so I think we're now done with that one and You know with my luck, they're gonna respond before I even finish the stream Okay, so this is a thing in cargo So cargo recently landed the ability to pass additional configuration option options using Cargo dash-config and you can give here. You don't have to give a file name You can give either a file name in which car in which case cargo is gonna read that file as a cargo configuration file Emerge it with whatever configuration applies to the build or you can give, you know dash-config Build dot target equals like a single toml key value pair And this is convenient for just you know ad hoc configuration where you don't necessarily want to change your entire cargo config You just want to change something for this run If both an environment variable and that are specified at the same time The environment variable overrides the file passed in over the CLI This is counterintuitive and also the documentation is unclear about this Right, so the documentation claims that Configuration value is specified this here meaning with dash-config Take precedence over environment variables. And so this is just wrong and I guess the steps are going to demonstrate this So in the cargo file We're gonna do Ha, so this is actually The steps here are problematic for a different reason, but let me talk through it first So the idea here is you set an environment variable that tries to specify this value This is a neat trick to know about For with cargo you can set basically any configuration option using environment variable Just by turning it in all uppercase turn every dot into an underscore and prefix it with cargo underscore So cargo underscore build underscore target underscore dur is going to set the build dot target dur configuration value in cargo And then cargo build dash-config dot cargo slash foo dot toml Actually ignore me I was going to say that this cargo is going to automatically load this file as well which gets weird But it doesn't because this is called foo dot toml. So the observation here is that one Configuration file is passed in here one one value set for this flag is passed in here a different one is passed in here and Based on the documentation. It's supposed to prefer this value because it was passed with dash-config but instead The build happens in from and rather than from foo toml, which suggests that this value is preferred And I agree this has straight up been correct I'm being CC'd because I landed I stabilized dash-config in the first place That's this or I didn't implement all of dash-config, but I helped stabilize it and Yeah, that's interesting. So actually the reason why this happens Is somewhat interesting. Let me See if I can't Explain what's going on here So in cargo There's a bunch of stuff that parses configuration files What specifically was this called this was called load file load values unmerged Unmerged load values unmerged Okay, what calls this? Yeah, so so this is the thing that Loads you see walk tree here. This is the part of cargo that loads the cargo configuration files from Dot cargo slash config.toml all the way up the tree from above where you run that cargo command and And you see it's unmerged because it just returns a vector of every value it finds And in addition to that there's So there's load values from There is Load file load includes. Where's did I end up adding this code? This was in include ha CLI args as table. So this is the part that parses the configuration options out of Out of the config args and you see here This is the part that if the argument is a file name that we load that file Otherwise, we basically create a single element tommo document and return that instead And I have a feeling we're gonna have to return to this line So this calls load file With this last argument being set to true right because it's a config include rather than one that was just found in a Normal walk through the stack And so the question now is What calls is CLI args? Merge CLI args That's fine. That just merges if you have multiple dashes config it merges them together. So that's entirely reasonable What calls that? Reload rooted at So that loads all the values from a given path. So this is what cargo uses to When it has decided where it's going to run from load all the config values config files from Dot cargo's has config from above where we are And in addition merge all the CLI args to cargo And this is the thing that allows you to set unstable cargo flags in the cargo config And so the question becomes Why that the config that gets set here? Why does that get overridden by environment variables? And I think I know the answer to that if I can find where That gets set from So it's like N Yes, I see here. This is the macro that's used to extract values get value typed and You see it gets so get CV grabs it from all the configuration that we've built up by reading things from disk Or from config values from the the arguments and get end Just returns that value from the environment if an environment variable by the appropriate name has been set And so here we see if Both are set like if it's both set in the config and in the environment Then if the definition is higher priority than the environment Then it returns the value from the definition Otherwise it uses the one from the environment. So the question is was this is higher definite is higher priority thing Let's see if we can find that Okay, so let's see if we can dig that up Is higher priority So CLI is higher priority than environment CLI is higher priority than path and Environment is higher than path. Okay, so there's one bit missing here, which is If it is a If it's a well, so so what's tricky here is dash config with a file name I believe gets recorded as a path. In fact, we can we can figure that out if we go back to this file And we go to the line that we were at previously see I knew we're gonna get here So fn load file does Here right CV from Tom all definition path and you see it uses definition path Even though it was actually loaded from a command line parameter So Really what we and and part of the complication here is this includes Argument. It's actually a little tricky It's false if we're you know walking the tree the normal way It's true when you use dash config in than a file name But it's also true for an experimental cargo feature that allows you to put include statements in cargo configuration files To basically embed another config in the in the cargo config and For those we wanted to inherit the definition that was already in place Right, so it's not just as though this should be definition CLI if this was true I think what we actually want here is that Load file here should set the definition as follows if this is not an include Then use path which is what it does today if it isn't include and it's from the CLI directly Then use CLI if it's an include and it's not directly on the CLI Then use the definition of the context we're being parsed in So this is where it gets tricky right so the way to think about this is If we're loading a path because of an include in another cargo configuration file We should inherit its definition because if it was loaded from a dash-config Then we should also be marked as being loaded from dash-config if it was from a path Then we should also be loaded from a from a path Now I'm not going to actually going to implement the fix for this right now, but what I'm going to do is instead comment on this Ha and you'd see way hung is already way hung is one of the cargo maintainers has already commented Well eventually can rate a config file definition path is the lowest priority of the three in comparison with the nbar occurs here Yep, so the nbar always wins over the config We may want to construct a config value with definition CLI here when load files triggering is traded by config CLI The doing that loses the path information of definition path requesting the error message, right? So so this is another good observation that So the the definition parameter to a config is What cargo uses to if there's something weird about the configuration to tell the user where that Configuration came from so they can go fix the problem If we assign it as definition CLI even though it was a file that was included then the error message is going to say You pass this on dash-config, but that's not really what was happening Pull request Some let's see. So this hasn't been reviewed yet. Let's go ahead and see what change they made Yeah, so it became an enum right instead of a bull And so load values unmerged went there Load unmerged include is file discovery. So I think this is actually wrong and load values from Uses and Let's see load file depending on why it was loaded sets the appropriate definition and load includes. Oh, yeah, actually, no, maybe this is doing the right thing. So load unmerged include calls Feels weird that this uses file discovery Because I feel like it might not be like if we go down to the thing that loads Uh, where is our where is our this one CLI args this table Load file Why load CLI? Oh, yeah load includes so the way they dealt with this is the The load includes takes a why load and so if we're in the path If we're in the regular file discovery Loading then we go through Loading includes when we path in pass in file discovery if we're in that Ashish config setting We'll we'll call load includes on any nested includes with CLI and then it is gonna load the file with that appropriate definition and This is gonna preserve the path even if it was loaded through CLI nice and now It's no longer be parsed as a path that's going to be passed as a CLI which which fixes the behavior But the path is still preserved for things like printing Nice And now, you know, this is where the old definition path was being injected Just always in load file and now depending on why it was loaded It'll generate the right thing And then it's gonna Nestedly load includes using the same reason why it itself was loaded. So you get the recursive case Yeah, right so the tricky case here is when a A dash-config of a file ends up triggering an include of a file and it looks like that's being treated correctly here You know the one one question here is Should cargo warn you about this right like in the in the original issue here of You know an environment variable, but also an explicit config flag Which one should take precedence should it really be the dash-config should that take precedence over environment variables? It's hard to say I I do think that in general like explicit arguments are preferred over environment variables, right like think of something like You know think of something like make so if you run n cc equals You know gcc make cc equals clang Install what would you which cc would you expect it to use? I think you would expect it to use clang And I think it would use clang um So I think the the precedence here is pretty sensible It does mean so someone pointed out in comments that if you run cargo by pointing it at the exact same file as it would have used Anyway, right? So if you do, you know by default cargo will look at Dot slash sorry dot slash dot cargo slash config dot tumble So if you did, you know cargo build dash dash config and passed in that path Then you're basically giving that file higher precedence in the hierarchy But I think that's intended right like you explicitly said use this file um So I I think that's reasonable. I think the the documented behavior here is is right Um, and you know now that we've gone through all this work of figuring out that this actually did what we expected um Let's go see whether we can just approve this. I don't have permission to merge cr merge prs and cargo, but we should take a look um So this is checking that It's a test that creates two config files and an environment variable It says If we set yes, you see this is the trick right cargo underscore k is going to set the same as k and the cargo config file um If we run that we would expect to get cli one So if you have multiple dash dash configs the ladder takes precedence and they both take precedence over the environment variable um What's the difference between the first test and the second test? Oh, this one is also including a path and then that path takes precedence over all of the above because it comes last And if you also give a key after the path that takes precedence nice um So so you'll notice this first one is a test just of the Config builder type that car uses internally and then this one is an actually integration test of you know cargo proper I believe She it's a good question. Yeah, this is using the config builder This is also using the config builder. So what's the difference between these tests? Oh, this is a test for the include feature specifically um So this is if we have a Dot config if we if we have a config that sets an include um Then we want to make sure that the included file gets loaded at the right priority as well That makes sense Uh Okay, so we only found one bug I think which is this say Priority Approve uh looks good to me Do one typo in the test name No Nope prove Nice So I think that means I can now mark this is done because I did did my work All right, I can close these again. I want to be back to now Um at error status enum and remove web driver error Okay, so this is we're jumping to a different crate again. This is why catching up when I get to notifications is often um I don't want to say painful, but often takes me a while is because at this point I have so many different repos that I have to Bring in all the context for each one not just because it's streamed, but even for my own sake I have to bring in the context or you know, repopulate my cache or something Um of why was this an issue? What what were you talking about? What was my last round of review? so Funtouchini is um A rust library for the web driver protocol that lets you control browsers programmatically And in particular It aims to be a relatively low level library. It doesn't try to you know be It doesn't have try to have like ergonomic fluent ways to do things It tries to map pretty directly onto the web driver client api and then the ideas you would use a crate like um It's one called 34. Like I think it's literally called 34 Yeah, I don't know why I don't want to call 34 but uh, it's the web driver library for rust and it um tries to be you know Really Nice like it tries to have an ergonomic interface um And it's actually built on top of Funtouchini It didn't used to not be but but now it is the idea being the Funtouchini provides like the underlying mechanisms And then this just provides the ergonomic mappings on top um, and this this person steve pride is the person who owns or Maintains 34 and so we went a lot of back and forth on whether 34 should build on up a fun Funtouchini what changes we make and ultimately um, you know, we we managed to to do some releases where it's now built on on Funtouchini One of the problems the Funtouchini has is that it uses the uh web driver crate Which is published by mozilla and you know, that's not a problem in and of itself except that the web driver crate um Tends to get updated fairly regularly So you see like every in fact, I think it's quite literally every three months they cut a new release and everyone is considered a breaking change and That means that at least in theory Funtouchini would have to cut a breaking release every three months as well In order to upgrade its version of web driver and the reason for this is because we have some of the web driver Types and we can look at what those are but basically the web driver um crate provides um That's a bad example uh command it provides Basically the the protocol type definitions for the web driver protocol doesn't provide any of the the mechanisms But but things like you know, these are the parameters to a get named cookie request to the browser and it happens to be the same crate that um What's it called marionette? The the firefox web driver implementation So that is the the thing that receives these commands and applies them to firefox It uses this crate to understand the things that we send So it's really nice to be able to just use something that we Sort of know is correct because it's being used by the other side at least for for some browser um But the problem is There are There are some parts of fantecini that actually exposes these types directly. Um, and one of the big ones is we expose web driver error Uh, and the reason we do that is because you know, sometimes we get an error back from The web driver Server that we're interacting with that we don't understand like it doesn't have a nice semantics like, you know, uh No such element or something. It's really just like an error code and a string and in those cases we We try to just return what it gave us In some cases, you know, there's an error status here that has a bunch of standard variants And we try to just propagate those onwards so that we don't have to replicate this entire enum in our crate as well Problem is again, that means that part of our public api Is part of the web driver crate and whenever that's the case It means that a breaking change to that crate would mean a breaking change to our crate in order to upgrade And that's becoming a problem We don't really want to have to do that because as you see web drivers are already on version 46 And it's just going to keep ticking up. It's not going to stabilize um And so what we've decided to do is after much back and forth Is to inline all of the types that we use from web driver in our public api into our crates so that we control them And that way remove web driver from our public api so that we're no longer tied to them for breaking changes And so that means copying over this error status enum that we just looked at remove the the re-export that we had in our public api of that type And Internally we can keep using the the web driver, but we need to map it to our internal Variant and there were a couple of other smaller fixes we made at the same time As you see, you know, we essentially copied over error status Most of the comments are public documentation from web driver spec Which is the other reason why would have been nice than not to have to copy them over because sometimes Their updates to the documentation would be nice if we just got that automatically, but unfortunately not Do we need to remove the comments that are not in the spec? I'm not sure what the npl license requirements are I think for npl, you're allowed to reuse but you have to mention the original project and its license And there was a couple of other things so for Fanta Chini we we used to have our own error enum That you know Lifted some of the web driver errors into nicer to access errors like no such element is so common You want to access it? But instead so we have this command error type Enum in Fanta Chini and we used to have one variant that was just This is a standard one that came from web driver And you know, no such element is a standard error type But we happen to just lift it into the upper enum to make it easier to access And with this change we're actually going to stick it back We're going to remove the the convenient access one and just have them all be under standard And it's a breaking change which is okay in the case of Fanta Chini We're already on I think I thought we were on an alpha. Yeah, I mean we're gonna have to do a breaking change anyway, but I'm okay with that because The hope here is that by doing this breaking change we can stop doing breaking changes whenever web driver changes Oh, it's 34 because the 34th element is selenium Which is the name of the one of the primary, you know originators of web driver nice Okay, so where did I get last I Made a bunch of comments 21 days ago. That's not too bad And Right so here The challenge is it used to be really nice if you try to like click an element Then the error type that we returned just directly had a variant called no such element now. It has a standard Which internally contains an error whose kind is no such element, right? So so now it's really annoying to detect whether the reason it erred was because the element didn't exist Um, which is what this comment is about, you know, like you would have to do this Uh, which is unfortunate. Uh, and so the proposal is what if we have, you know, uh helper methods that just Quickly check this for you without you having to do all these matches um And it looks like I asked for that and they've added that I don't know what these are Okay And That's fine. All right, let's go Look at these changes Files changed I think there's a way for me to say changes from Changes since your last review So get those first And a to hide annotations Uh What's this Right our web driver error type is Oh, this is just a simplification we made to the constructor. Let me see if I can find that down here somewhere new yeah, so The error type we have Can either hold a static string reference or an actual string depending on whether you have a, you know, relatively standard or a custom error message That you have to like dynamically construct And the way you represent that is with the cow type which can be, you know, either a reference or an owned value Uh, and we just said that the the lifetime of that reference has to be static if it is going to store a reference um And you know, that makes the api a little awkward because now you have to like Either call into or specifically bring in the cow type and use the appropriate borrowed or owned variant um And so the proposal I had was let's use simple into because both string references and Capital s strings implement into cow static stir And so this means that now we just stick the into into new and it means that any call to it like the one that we saw up here Um no longer has to use into It'll just do the right thing um No such element is now gone as a variant and it's it comes through standard What was is miss What why did I name it is miss? Weird uh Is detached shadow root? um I'm torn here like, you know, this seems maybe excessive But I guess we might as well just have one for each That seems fine It'd be kind of nice if we could generate this with a macro, right like this is a lot of Repeated code that's almost the same um So what i'm going to do here is actually um Be nice if we could turn all of these nearly identical definitions into a macro something like um let's see if I can Cook up one of these on the fly Um Is helper um, and it's gonna take Here's what i'm gonna do I'm gonna write a little bit and then i'm gonna explain the syntax um It's gonna be Actually, we're gonna do Something like this maybe or maybe that can be It can't be a type it can't be a path might be an indent actually um And then we're gonna do So this is what we want to generate So here's what this macro is going to generate It's gonna For each element So the syntax here for the argument list is uh The the outermost bracket doesn't matter. It's just a it's just required for the syntax. The next one is When you call this macro, I expect there to be a literal curly bracket to start this next uh ooh This next dollar parenthesis Is saying this is a group and you see it it ends with comma star which is saying this is a group of multiple elements Zero or more that are separated by comma. That's what that syntax means. So star is zero or more comma is separated by comma And I don't think I actually want that anyone um and The things that there can be zero more of are Uh ident so ident is any identifier. Um, this can be things like variable names function names type names um So it has to be some identifier followed by you know Fat arrow followed by some other identifier and the the syntax here is Dollar then name of the meta variable. So the thing that you're going to use as the the substitution in the uh, what's this called the transcriber um So in this case the first one i'm going to call variant It's going to be mapped to variant the only one's going to be mapped to name uh, and then What I wanted the could I want to generate? Why can't I there we go? That's what I want uh And then what I want here is actually uh this Should be name This should be variant And so someone asked the the very important question here about can macros generate doc comments and The answer is it's complicated, but Yes, um But they can only do it in a very particular way which is Um I wonder if this will work and you're gonna hate me for this I think it has to be like this and then So here's a secret you may not know about rust, which is that um Triple comments are actually attributes And they're attributes that read doc equals right rust Reference, let me check that i'm not lying to you um Where is my comments Dot comments a line dot comments beginning with exactly three slashes Are interpreted as special syntax for doc attributes. That is their equivalent to writing doc equals and then a string uh And so Here's what you can do You can say doc equals dollar variant And I think it's like Stringify I think it's that I mean, okay, maybe maybe I should actually do this somewhere else to see that i'm not completely lying to you um Do I have a temp in here great cargo new? expand CD expand Uh, and it's gonna yell at me somewhere Who knows why yet? I expected one of right. So it's this is for the repetition. I need to say star again Uh, and here it's complaining because that has to be a semicolon. That has to be a semicolon Where's the semicolon have to be? um And then if I now say, you know enum Error status or I guess I'll have to do, you know enum command error And it's just kind of a standard. It's just going to be an error status Uh an enum error status is going to be foo and bar And then what I would like to be able to do right is I would like to be able to do um Imple command error And I want to do like, uh Is helper And in fact In fact, I'm going to remove this extra one Because it's not technically needed Because you can use any type of bracket for invoking a macro and I'm going to say, you know, is foo or sorry foo is going to map to is foo Bar is going to map to is bar That's what I would like to happen Uh, and I think it doesn't allow trailing commas. That's fine. Uh, we can do that by just saying uh The question mark is zero one. So I'm saying zero one comma at the end So we're actually going to do this Errors error status And it's still mad at me. It's gonna it's going to auto format once I fix whatever it's complaining about here Is it a tuple or a struct variant? Is it a struct or a tuple? Uh, fine Uh, then this uh, this is like a web driver Error And so there's a struct web driver. The only reason I'm typing these out Is I want to make sure that I can just copy paste the macro at the end and it'll it'll generate the appropriate, uh, setup Um Equals cannot pass for error status. That's fine. This is going to derive Uh Partial eek and eek And remove the semicolon Consider removing this semicolon Uh, and it's going to generate pub. That's fine. We're going to just make all of these be pub so that it doesn't yell at me unmapped unmatched end Of that, where did I mess up somewhere? I'm not quite sure why it's complaining about this. So if I now run cargo expand Let's see what it generates This feels like it generated correctly Okay, so I don't it's just a Rust analyzer being confused. You see this is the expansion of our macro So that worked now one question is going to be does this actually You know generate the right thing um Right because I'm not sure whether when we Parse this markdown it's going to end up adding like white space or something. So if I run cargo duck No depths Uh That's just open nice If I go to command error Yes, you see it it adds a It adds a white space here Which means that the link isn't going to be valid It adds a white space before and after Mmm Yeah, so I think we can do Concat I think we can do this And in fact Yeah, technically we can do this It's nice to keep it relatively self contained Um, I have to put the the dot in there because otherwise um The next line is going to be this is going to be a space before the period But now if I do this what do we get commander? And that links appropriately to the right variant Nice So that means this macro is To borrow a great term the shit So let's go ahead and do this And then do You can then replace all of The methods with something like this Preview nice Uh started review Yeah, so this is a a a good example of the kind of things that I really like macros for it's just like it it makes Anything that looks really like a repeated pattern. It can make it so much more concise, right? Like reading this This is much nicer Okay, and then These we can all move past Seems fine Yeah, so this is a a thing I asked for which is um For all of these error status variants, there's a in the standard there's like a String description for each one that you're supposed to include whenever you trigger it and because it's statically known it's It's just good practice to actually return it as a static string because who knows it might be useful to someone When you implement error you get to define the description, right and usually that's through the display trait The problem with display is it doesn't give you a static string or rather. It's not a problem There's error description used to require this and it was really annoying But so here what we're doing is we have a description that someone can use to get the static string if they really want to um and then Display just writes out that description. So we implement display, but if you really want the static string you have a way to do it um And that way when we serialize we don't have to convert it into a string and then serialize the string We can serialize the static string instead and save an allocation um And this into cal we already talked about Uh, it seems fine This now becomes Nicer because it can just use the helper This becomes nicer because it can use the helper can use the helper can use the helper I'm guessing there are a bunch more of these And these have just check annotations So the one thing i'm curious here then is was there anything else One thing that that annoys me a little bit about github's, uh, UI is that Like I kind of want to see the diff like from Including this one And I also don't want these to be resolved because I want to check that I have resolved them not just they have resolved them um So for example, I asked to derive hash and ek and I'm guessing that's done in this commit um Right, so this removes the The sort of hoisted variants and stick just leaves them in standard Um, that's fine that goes away in the next commit like we saw Um This is an internal converter that turns a Actually, this shouldn't be necessary anymore now because I guess it just wraps the standard. That's fine Um, those are no longer hoisted this now derives ek and hash Which is fine because error status, you know, who knows where people might want to stick them They can't be ordered in a meaningful sense, but by having them implement ek and hash you could for example start them in a hash set Uh, might be useful. So that one is indeed resolved This is It's weird that it's called an error code, but it returns a string um, and It is indeed that's now been turned into description like we saw and an implementation of display So that one is indeed solved um Yeah, so this was another one. Um, it used to be that there was a implementation of from The error status type in the web driver crate for our implementation are, you know copied Version of error status and that's not okay because that this impulse is a part of the public api So someone could in theory be, you know, taking the dependency of fontecini and the dependency on web driver And relying on this from implementation working for the particular major version of web driver that they are using Which would break if fontecini ever upgraded its version of web driver Because the from trait will be implemented from a different version of the error status type Um, so I think ultimately that was removed. I hope Uh Yep, that was taken out. That's great. What else do we got here? Uh, this is the ability to Parse an error status from a string. So it's sort of the it's the inverse of turning it to a string Uh, and I just said that should be basically a parse, you know, there's an argument for it's a little annoying that we have to define We basically keep the need to keep these strings in two places. It means they have to be kept in sync Um, it'd be nice if we didn't have to do that Um, there are macros that let you do this sort of two-way conversion Uh, we could write a trivial macro to do it ourselves right to to generate both the description and the parse in one go Um So actually let me I think that's what I want to do. I think I want to go here Look at the files changed Um Look at this is in source error and over here Get rid of the annotations I said get rid of the annotations. There we go um But here too it might be nice to not have these string literals, uh written out To in two places in the code Uh here and in fromster um May be worth turning them into um Maybe worth turning them into constants Or writing a little helper macro to generate Both this method and the froms and the impo fromster at the same time Uh, great. What's this one? Uh The message okay, so where does that come from? So try the difference between try from and from is that try from can fail Um, it can like return an error if the conversion didn't succeed Uh, this gets to now store a static. That's nice If the thing is static Uh, this was an unrelated thing that got resolved without a change And there's a Test somewhere further down here That now checks no such element. No such alert. Yeah these we all looked at and this one Is a stale element reference So I think this just used to be wrong interesting, okay, um So at that point, I think what I want to do is actually So these all look like they've resolved So I'm torn here because you know neither of these are really blockers Uh I've left two comments that are more Suggestions for Uh improving the niceness Of the code, um, but I wouldn't consider either a blocker if you Want me to merge this as is Um, I'm happy to do that as well Looks great now positivity always inject some of that people are helping with your software Uh, great submit review Done Okay Well, we've got we've gotten through a whole three things Uh, okay Uh native sassel support, okay, so this is for a different crate entirely this is for the imap crate Uh, this is why I need to give away ownership of some of these crates soon Uh, let's see. Yeah, so this is for the imap crate and it's specifically The imap protocol has multiple ways you can authenticate with a server The most obvious one being, you know, you you give a username and password um But there are other ways to do it and there's in fact a A protocol authentication standard for how you can have different authentication schemes Um, and that's known as sassel I forget what sassel stands for It's like something Sassel simple authentication and security layer Sorry for the bright screen, um and The way that the imap crate has traditionally done this is it has an authenticator trait. So if we go look at, um Yeah, my crate Which has an alpha and has been has an alpha for for 3.0 for a while now Um, partially because of things like this like I want to land that because it's worth using. Um, so the authentic The authenticator trait, um, all it really requires that you implement is a Process method that takes a challenge from the server and it returns a response that can be written back to the server as bytes Um, so it's very straightforward, but authenticate wasn't really written to specifically work with, you know sassel like it doesn't It's a very manual thing to have to implement right bytes to bytes um The current authenticator system most of sassel can be implemented. There are a few advantages to using an external crate so the idea here I assume is that there's a uh There's a crate that specifically does sassel Through I guess a library that does it for you um That supports, you know, whatever multiple different authentication standards um Rather than requiring people to implement this authenticate trait themselves Which you know can be really complicated for some of these authentic authentication standards. They're not always trivial to implement internally um future like Sassel security layers and channel bindings aren't really doable with a current approach. I don't know what a channel binding is Sorry again for bright screen. I don't think you can turn rfc's dark um Interesting Oh, I see so here the idea is that you can have the tls session Be the thing that authenticates you and so that way you don't actually have to do any authenticate But you do need to tell the server that you wanted to do that um Interesting Let's see what the diff they're proposing. So basically what this pr is asking is like hey, I want to make a change that's sort of like this um Would you be okay with this kind of a change basically? Tying imap to this particular sassel crate um Rather than you know the current authenticator api So the first problem of course is that this is a get dependency which we can't have if we're going to publish to crates.io Which is fine like i'm guessing that's just because they're they're doing development and so they wanted something easy to bind to um So this allows you to create a sassel config Right, so you create a client builder You can start to sassel config with whatever stuff and you call client authenticate and you give in a sassel config in a mechanism Yeah, so instead of calling dot login So if you look back at At the api um iMap has a client in a session So a client is just you're connected to an iMap server and in general You're going to call login in order to turn that just connection into an authenticated session that lets you access email So you see it returns a session if the login is successful Or you can authenticate using an authenticator And here the proposal is that authenticate instead of taking this or you know our own type here It takes a sassel config My first thinking here is these don't have to be mutually exclusive right one option here is that we could have a A dot sassel that takes a sassel config if you want to authenticate with sassel But if you don't if you just want a relatively simple challenge Challenge response scheme for example or for testing actually this might come in super useful. You can use an authenticator instead And once you authenticate you have a session and then everything works the way it usually did So let's see it removes authenticator, which is really just the definition of the trait I think It now brings in our sassel and of course we would want to bring this in under a under a feature So authenticate now takes a sassel config why an arc sassel config And what's sassel client here? That seems weird. I don't know why this requires that it takes an arc Um interesting Yep, so it passes the mechanism here Then it does an off-hand shake using the sassel session where previously we were like manually parsing the Basically the binary data that comes from the server You know turning it into bytes by base 64 decoding Sending it through the authenticate trait that we were provided and then base 64 encoding the response back to the server And this has to do the same thing like it still has to do the decode But when it has done the decode We're just parse Parse authenticate response. Where does that come from? Oh, I see that that already exists. That's mostly the base 64 decode. So what does it turn things into? After it does the decode When it has the data It'll do Authenticator step 64 Yeah, so I think this is you know you give it You run the next step of the authentication process then there might be multiple um And so here we're going to step 64 with the challenge that we just decoded Allow it to write back to the output and then write the output back is as u64 And once we're done We tell sassel that okay, there's no more data come from the server. Just you know finish authentication So overall this this looks pretty reasonable to me. I think the Um Okay, let's let's do this. Um Let's see Reasonable thing to add Though I'd want to add it Behind a feature flag I also think we could do it in an additive way that is Keep the current Authenticate based A Authenticate based api. Um, and then provide a sassel authenticate method uh Called say what do we call sassel login? sassel auth maybe um The sassel Future is enabled that does auth through sassel Uh authenticate may still be useful on its own um For users Who are working with relatively simple or experimental apis? Mechanisms uh Such as during testing Um One thing that surprised me when Uh glancing over the changes Was the need for arc wrapping the config Can that be avoided? Why is that required by the sassel client api? Great comment Done, uh, what we got next? Oh, yeah, note about the git dependency good one. Um Oh, also we'd want to make sure by the time this lands uh that we Can take a regular crates.io dependency on our sassel Rather than a git dependency so that we can in turn probably Continue to publish imap on crates.io Uh add interactive mode. Okay, so this is a prd or a different project which is rogit. Um, which we used for Uh, this is our wordl solver. Um, and Someone submitted a pr that adds an interactive mode and by interactive here I don't mean playing the game. I mean helping you play a game the idea here being You're on wordl.com And it's asking you what's your guess you can run rogit in interactive mode It'll tell you what you should guess And you have to type back in what the real wordl told you the correct and wrong Answers were like basically which ones are gray which ones are green which ones are yellow Put that in and it'll tell you what thing to guess next Basically, how can you use rogit as a wordl cheater? Uh, if you will Um, and this is a surprising amount of you know back and forth You're trying to figure out where's the right place to land this like how should we change the api to enable this kind of interactive mode Um, ultimately, I think we got pretty far Um Let's see. So the ask I had was Right. So one challenge here is you know, the the ui is pretty minimal here um And it's complicated because you know, it gives you a five letter word and ideally you want and this is what the The person's saying in the response here when you input your Your the correctness of your guess that you get from wordl You kind of want your response to a line up with the output from the program that told you what letters to guess Just so it's easier to like visually match the two What I was saying in my comment was It's not obvious that you should use like c for correct m for misplace and w for wrong Right, that's not clear from you know, if it just says colors, which is what the prompt is here um It is true though that it's night if they line up, but they wouldn't if the prompt is as long I think one option here is to have the this prompt appear further up um That's a good point um What if we print out these instructions further up in the output Print out these instructions just once further up in the output like before we Even print the first word to guess Where we can have longer uh description of a longer description Of the expected input without breaking the alignment um also colors is a little weird here given that we take cmw as input not gy not green yellow gray Now these were just some minor suggestions to the error output Oh, yeah, so this is a fun little change. Let me see if I can See that in context, um Here maybe line 155 So they had this this is a really useful thing to know about Oh, how can I see this before they force pushed? Here maybe so they had this um Loop up here where they're walking all the overall the characters of a string They're map trying to map each character to the appropriate like enum variant So they're basically parsing the string one character at a time And if if any of the characters is the wrong character or rather one that we don't have a mapping for They produce an error then they collect all of those into a vector And then they look whether any of the things in the vector is an error and if The thing then then they um Get the thing at that location in the vector and they question mark it to turn to propagate any errors um All right. So this is basically You know find the first thing that is an error Get it and question mark early returns, which is why this is unreachable to propagate that error Uh, and then because they've done this they now iterate through all of the things that are parsed Map them to unwrap Because they know they're all okay because there's nothing here And then they collect it into a vector again, and then they try in to turn it into a five length array So there's a really like convoluted way of saying I want to just error if there's something bad if there's something that's not an appropriate character and This is where it comes in really handy that you actually have the ability in rust to collect into a result of a vector Which made that whole code makes it a lot simpler because now you can just do the same thing we did right So match and produce the appropriate okay or error collect into a result veck which has the semantics of Keep collecting into a veck or keep collecting into an okay veck rather But if you get an error from the iterator then You know get rid of the whole okay veck and instead just return that error And then we can question mark that and that gives us back just the vector and propagates an error And then we try in to turn it into an array. So it just it just makes it a lot more concise um So that was So this has been fixed This has been fixed That was my comment here Uh, which they've done and then What are these diffs? These are probably just rebases I think this is all just force push Yeah rebased on top of main Um Let's go look at the Changes so I left the one comment here that was just you know about the prompt um These conflict with each other that seems fine if interactive the play interactive Play interactive takes a guesser Oh, they they've added that nice. Ah, you already added Uh Instructions at the top nice, um What is the actual This is a coverage failure I feel like I've probably broken coverage. So that seems like just a separate problem. All the others are fine. Um I still think Colors is a little weird, but let's not block on that Start review finish review approve And merge Thanks for sticking with this Excellent Done update dependencies A hash and quick xml So this is in a different crate again. This is an inferno, which is our port of Flame graph to rust Um that updates some dependencies in particular A hash and quick xml and I asked them to also update criterion Um Yeah, so for inferno, we bring in a bunch of test cases from the flame graph Perl implementation, which means that we have a git sub module that Holds all the the data files for that And so if you don't do git sub modules update dash dash in it Then you don't get doesn't download the sub module for you and so the tests are going to fail Should arguably add a build script or something to make that nicer Um, all right, so let's go look at the diff here Cargo lock, that's fine cargo tumble That seems fine. So that's been viewed. Let's just mark that as viewed too That looks like a clippy lint. That's fine This is a quick xml change Of now it just has a new instead of owned name and borrowed It's interesting. So if we go to quick xml And we look for new So what was this byte start? Yeah, so it's now an into cow. So that's the reason you don't need to separate borrowed and owned anymore So that seems pretty reasonable And that's probably going to change a bunch of things in the Internals here No longer lead from plain string. That seems fine It's all fine Decal Oh nice, so we don't have to Raw print these this is now a xml declaration version of one standalone No It's a dock type where we just keep the Same dock type as before. That's fine Right svg, this is more things with attributes That's fine It's a change to new change to new Wonder why this is from content. Oh, this is um Because this has attributes as well And so from content also parses the attributes So let's go back and check that I didn't mess up This anywhere else These are all just attributes. These are all just um tag names so they don't need from content Same here same here. Even even if they did one of the tests would fail. So that seems reasonable um Okay this is byte start New stop With attributes this iterator has just been rewrapped a little bit otherwise it looks fine the same with stop Same with this this looks fine looks fine Oh, hey a bot that I can ban block user nice love spam um From escaped from escaped from escaped Yeah, so these all look like pretty straightforward api changes new New that's all fine Uh Oh, this is also a clippy lint of if your pattern is a single character then use a character instead of a string That's fine. And then this is all changes to the The expected test output I don't actually know what this is going to render Really, it's not going to let me look at the rich diff. That's fine. Yeah, these are all just svg changes to the tests Uh, which seems fine And there are no more than that great And so the next question is this criterion a hash or quick xml in our public api. I'm pretty sure they're not um Also, it wouldn't really matter if we had to do a breaking change for inferno, but Well, it matters a little bit Collapse as the collapse trait which does not have any quick xml in it And these just implement collapse that's all fine all these are just options So these aren't going to have anything in them differential from files just takes options from readers takes options Nothing in here flame graph Has from files. These are just going to be simple. This is an enum This is an enum options has lots of fields But none of them are from a hash or quick xml same with this Default there are crates that help you do this. So there's a Um The cargo public api command and the semver check api Or command but both of these help you do this. Um, in this case, I'm pretty sure there's nothing like, you know It would be weird for us to expose Um Like a hash or quick xml in our public api. We do expose rgb, but that's fine Um color multiple at basic plat. Yeah, all of this is very Very non-public Nice Excellent. Thank you Approve Well, here's a problem. I can't I can't scroll down enough There we go Um great to prove and run Hopefully that those tests are gonna pass While we wait for that, I'm gonna Take a bio break. All right. I'm back. Let's see Why are these still some of them are failing seems unfortunate Uh, this is cargo format complaints. This is Feature may not be used on the stable release channel In the ito a crate Oh interesting. This means Oh, this is from the minimal versions Okay, so let's talk about minimal versions a little bit. Um, one challenge here is um I run one um Test step in most my ci that puts all of my dependencies at the Lowest version the lowest version within the summer range. That's allowed by my dependency closure The idea here being that you know If I did require some newer feature I want to make sure that That's declared in my cargo tumble And the reason I want to do that is the failure mode is kind of weird if someone has imagine someone built my project ages ago And so they have a lock file from that build Then they do a git pull and then they try to build If my minimum dependencies aren't declared correctly, then they're going to get a really bizarre build failure Because they're using an old version of a dependency That my project doesn't work with anymore, but I haven't upped the dependency and so therefore I they don't get to know Uh, that's why it's it's just nice to do that check. It's not really a requirement, but it's nice. Um now What this is running into is I guess the by updating some of these dependencies probably quick xml. Um We also got a new version of itoa and The update to minimal version step that I do is going to upgrade to the lowest version of itoa within the summer range Which is going to be version 0.4.0 now, of course the Build that we do runs with some particular version of rust and it runs with a relatively recent version of rust When itoa 0.4 came out or was building um, I guess it was using a feature It was using a nightly feature when a particular feature was enabled Which is a little weird already But it has since been stabilized on newer versions of rust So what this is complaining is first of all feature may no no longer be used on the stable release channel But also that feature has been stabilized so you can use it on stable now Um, there's not a great way to solve this particular problem, right because the itoa dependency comes into us Um, actually it might come in directly. Let's go see if that's true Uh cargo tumble We have a dependency on itoa 1 Where is itoa 0.4 even coming from? That's also interesting That's real curious Uh, I'm going to guess I have no idea why any of these would Make it so that we get in, uh Such a low version of itoa Okay, here's what I'm gonna say, uh for the There was something else. There was a failure down here too Trade bounce other than size on const fn parameters are unstable. This is with quick xml 0.25 That's interesting. So what is quick? So for quick xml, what's its actual version? This is the thing that crates that I always started doing for me, which is very annoying Which is if I try to go directly to a crate I'll just load forever unless I delete my cookies for that page um Huh quick xml 0.25 Because this is just a build error in quick xml On line 64 of source writer That's interesting source writer Line 62 const of yeah here this commit Drop const of n Because it basically requires to use a particularly new version of rust Yeah, and you see it actually fails on rust 159 And so this is another ci step that I run Which is I try to make sure that the crate continues to build with a somewhat older version of rust There's a lot of debate on the rust community about you know, which versions should you support? Should you even bother trying to support old versions of rust? um I like to do it at least on a you know, best effort basis And so this is basically saying it fails to build on 159 and it fails to build because quick xml basically in in their zero, I guess zero 24 release added A const fn For a generic function, which doesn't work on rust 159 Apparently it only works on newer versions. I'm guessing this pr will have more information It does not But it does add a ci test now to to see that they don't regress on this And I'm guessing there's been no release of this Let's see uh Right as though there's no actual release for this yet Uh, so let's go ahead and say So what we're going to do here is um Looks like we have a couple of ci failures The minimal versions One seems to be because we somehow now pull in Itoas 0.4 dot zero Which is Super old um Not sure why If you run cargo update dash c Minimal versions and then cargo Tree dash i itoas 0.4 point zero um You may get some pointers to What's happened that said this Uh, I'm fine merging even if that one is red It Could be that uh rebasing that merging from Main will fix the problem for you um The msrv ci step 1.15 9.0 is because of We link the actual issue. I guess maybe there isn't an issue um Uh is because of a msvr regression in Quick xml It's been fixed upstream But there hasn't Been a release yet um I'd actually like to hold off On merging this pr until that lands um at which point we should bump the Quick xml dependency to 0.25.1 Uh, and then the check stable format ci step seems to complain about a The lack of Cargo format being applied to align It changed Should be an easy fix Comment Great all right, uh I think with that I can now mark this as done And I think with that we're Let's do one more. We'll do this as the last one um Okay, so this was a longer discussion basically Um one of our dependencies for this is for a different craped every single one We've looked at has been a different crape This is for the open ssh crape, which gives you a sort of programmatic api for Doing ssh operations and sshing to machines and running commands there And we have a dependency on a crate called open ssh mux client Which is instead of interacting with ssh through the or open ssh through the ssh command We actually directly implement the protocol that The ssh command uses when talking to An ssh command master control demon That you can set up to reuse ssh connections We do that through the open ssh mux client crate The problem we have uh, and as I pointed out in here, um We actually expose and this is related to what we did with fantaccini We have one particular error variant in our crate Um That over here that you can see so we have a we have an error type in the Open ssh crate, which is public and one of its variants is An open ssh mux client error that you can sort of propagate up And this one You know means that the open ssh mux client error type is in our public api Which means that if there's a breaking change to this crate we have to do a breaking change to our crate And in this case, this is a breaking change to that crate So if we did take this upgrade from dependabot, we would have to do a new major version of open ssh as well Um, and I think the the maintainer of open ssh mux client who is also a contributor to open ssh Didn't realize that this was the case and so sort of did this breaking change without quite, uh, realizing the implications And so Yeah, so this is one of the tools semmer checks that the tries to detect whether something is a breaking change And in this case, I guess just it missed the fact that this wasn't the breaking change Uh And so things got sad Um, so ultimately They released It turned out that this particular change that they made was actually not a breaking change to open ssh mux client So they released a new minor version of that rather than the new major version, which we then just bring in automatically Um due to the release of ssh format 013 the next release might have to be 0 16 0 1 which would be a breaking change Uh, they're going to try to group together a bunch of different breaking changes once. That's a good idea Uh, okay They move all the aerotypes into separate crates the mark them is non-exhaustive. Well, this fixed the problem That's interesting. So the proposal here is if we made the Error types of open ssh mux client being a separate crate. So a crate called like open ssh mux client errors, I guess um, then And we make the Error enum variant or the error enum in that crate be non-exhaustive so that we can add new variants to it Over time without it being them without adding the variants being a breaking change Then our crate could take a dependency on that crate And now the open ssh mux client crate could keep doing breaking changes and it's no longer part of our public api Uh, and there wouldn't be breaking changes to the error crate and therefore we wouldn't need to do breaking change So that's an interesting proposal the It's a little weird to have a crate just for your errors But but I also sort of understand the attraction um I we basically have We have four options here. One is to do this have a separate crate. Uh, the other is to try to remove The crate entirely from our public api which would mean basically Not exposing those errors like basically you could imagine, you know that we could turn them into a You know a string or something there's a variant of that. So the the third option we have is to Expose that error as a like a box didn't error Uh, which would mean that we're no longer tied to it and people would Consumers of the open ssh crate would have to like downcast them into the appropriate open ssh mux client error type Uh, and that would work like it would it would mean that it's not technically a breaking Change for us to bump open ssh mux client But at the same time it would break our users nonetheless because the downcast Would start failing for them if there was a breaking change because the downcast would be to a Different type because it would be different version of that type um And then the last Uh potential solution here is um We could inline the error type basically keep a copy of that error type in our crate And then you know basically do the same thing as we did for fun to chini with web driver It's not very attractive here. Um, especially because you know It means we have to keep up to date with the changes to that error type And I don't love the idea of having to keep a separate copy I actually think I kind of like the you know keep errors in a separate crate proposal here Um, it's a little more cumbersome for the people who maintain That crate but not really for us. I guess if they're in a workspace is not that bad um Yeah, it's a bit more um In convenient for you in maintaining Open ssh mux client But it would solve the problem as long as um But it would solve the problem as long as Uh, you don't have to Make breaking changes to that error crate Keep in mind though, and this is an interesting point keep in mind though that um if the error If the errors In that crate internally contain errors from somewhere else like another crate such as ssh format, uh, and The error crate needs to update its dependency on ssh format to propagate new errors Then that would still be a breaking change unless um the new Version of ssh formats Errors go into a new variant and both Uh The old and new versions stay Around as dependencies I don't know what kind of dependencies You're taking In your errors though May not be a problem in practice it would certainly reduce the likelihood that breaking changes in Open ssh mux client would require a breaking change in Open ssh um Another option here is to type erase the errors and store them as boxed in error plus Uh send plus sync plus static Those extra bounds is because we want them to be sent in sync in general You want errors to be and you want them to be static so that they support downcasting um Type erase the errors and store them as that um Which would technically allow us to get away with not doing a major version bump for Open ssh when open ssh mux client does one Although in practice It Could still cause problems for user for open ssh users who rely on downcasting Although how many of those are there really cause hard to debug problems comment Oh, yeah, someone made the the point in Chat and this isn't about this specifically but um in general Try to keep your pr is somewhat targeted because it makes it a lot easier to maintain Otherwise you end up with like can you fix this thing and also this thing and also this thing that there's a lot more churn in the pr I would rather have many small prs that I can land independently um Someone made the point in chat here that this is an they write uh This is an odd symptom of the weirdness of rust's error handling story all the options are kind of unsatisfactory and I I don't know whether it's about rust right like I think this is more about The implications of semantic versioning right like if the if the only toggle you have is Either the whole crate had a breaking change or the Or the changes in entire backwards compatible You can't do things like you know individually version your types If you could then this problem would sort of go away um But in reality what's happening is here. This is more about strict typing right where If you really looked at the types they did change right like this is a different type um And if you want the compiler to do type checking for you You wanted to warn you about these kinds of things at compile time because if the type is different The semantics may have differed uh or different fields might be available So like I I don't buy that this is like an Unnecessary problem. I think it's just a hard problem And one way you can get away with it is sort of you can you can do duck typing here right of saying um Which is basically what the the box din solution does of saying all we're going to store here is this is some kind of Error and not the concrete type and then you give access to things like you know it can you can print it um The the way you get at this with things like you know in ruby for example, you can throw an exception and You know, there aren't really some different Versions of a dependency If something has roughly the same shape Users are going to keep being able to use it as as long as they only access fields that exist in both the old And the new version of that shape It doesn't matter that the type is actually the same But but in rust that's not true because we have strict typing and the The error context api might actually help a little bit here because it makes it more Defensible to use something like a boxed-in error The context api is basically, you know a way to say On this type give me a field that has of the type u64, right or of the type I don't know, you know the of the type some type and you can't use strings with it I don't think you can't use like keys. It's only by type But that would mean that it might not matter whether you can downcast the error into a specific known concrete error type But rather can I access this kind of context through that error? And so that means Basically your the users of your library are less likely to rely on Can I downcast this into this version of this concrete error type and more likely to just rely on Does the the opaque error that you gave me does it have these properties that I care about? But it's still not perfect. It's it's definitely not Um Yeah, so so you have the um December hack for this where Instead of having a separate crate for error Um, you have the new the old version of the crate take a dependency on the new version of the crate And re-export its error type and that way the the types are actually from the same version and things just continue working um It would have the same property here um I don't know if it's better Right to keep it in a separate crate. I guess we could we could um December hack December trick is what it's called Oops, that's not what I meant um You Could also use the semver trick Uh specifically for your error type when you do A new major release and it would have the same effect as um As having a separate error crate Although it makes it harder for Uh users to Realize that they can rely on the backwards compatibility of The error type and only the error type Uh across major versions versions Uh great done All right, I think we're gonna stop there. We got through one page and there's another page But it's getting late and I need dinner Um, but thanks for joining. I'll I'll see when I do another one of these, you know My notifications keep piling up. I have two new notifications since we started the stream So like there there is always more work of this and who knows the next one will probably be impromptu as well Um, hopefully that was useful. We went through so many different repositories Um, but thank you all. Uh, have a great rest of your day or night depending on where you are And I'll I'll see you some other time