cellars: write tag specific cellars

With #10186 now merged, the tag specific cellar information is being read by brew.
This PR (once merged) will start adding the cellar information for each tag instead
of having a single cellar line on the top of the bottle block.
Each new CI build in homebrew-core will slowly start migrating the cellar lines to
the right place. If keep-old is used, the old "all tag" cellar line is removed and
added to each tag.

[ ] Have you followed the guidelines in our Contributing document?
[ ] Have you checked to ensure there aren't other open Pull Requests for the same change?
[ ] Have you added an explanation of what your changes do and why you'd like us to include them?
[ ] Have you written new tests for your changes? Here's an example.
[ ] Have you successfully run brew style with your changes locally?
[ ] Have you successfully run brew typecheck with your changes locally?
[ ] Have you successfully run brew tests with your changes locally?
[ ] Have you successfully run brew man locally and committed any changes?

Review period will end on 2021-01-21 at 22:57:09 UTC.

Review period will end on 2021-01-21 at 22:57:09 UTC.

Review period ended.

Review period ended.

Note: this can only be merged once a new release of brew has been made; because the reading part of this needs to be shipped first.

Note: this can only be merged once a new release of brew has been made; because the reading part of this needs to be shipped first.

Cool. If everyone is fine with the current implementation, I'll merge this just after the next homebrew release, when I have time to monitor the next merges and react to any issues, but I think with all the tests we have in place now this should work out of the box :)

Cool. If everyone is fine with the current implementation, I'll merge this just after the next homebrew release, when I have time to monitor the next merges and react to any issues, but I think with all the tests we have in place now this should work out of the box :)

My suggestions in commit form: https://github.com/SeekingMeaning/brew/commit/28bb6cd8d

My suggestions in commit form: https://github.com/SeekingMeaning/brew/commit/28bb6cd8d

💚! @iMichka feel free to merge when you can be around to babysit this 😁

💚! @iMichka feel free to merge when you can be around to babysit this 😁

Cool, thanks. I'll do that once we have a new release with the reading part, else we will break everything for our users.

Cool, thanks. I'll do that once we have a new release with the reading part, else we will break everything for our users.

I added one more commit (40ad563279c109753f1e123cfc8eba7e5a9da1be) to skip the "line too long" message for the sha256 lines in bottle blocks and rebased.
Now that brew 2.7.8 is out, I'll merge this one later today and monitor how things go in homebrew-core.

I added one more commit (40ad563279c109753f1e123cfc8eba7e5a9da1be) to skip the "line too long" message for the sha256 lines in bottle blocks and rebased.
Now that brew 2.7.8 is out, I'll merge this one later today and monitor how things go in homebrew-core.

I added one more commit (40ad563) to skip the "line too long" message for the sha256 lines in bottle blocks and rebased.

Can we wrap this instead e.g. from:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur, :cellar => :any_skip_relocation

to:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur,
       cellar: :any_skip_relocation

I added one more commit (40ad563) to skip the "line too long" message for the sha256 lines in bottle blocks and rebased.

Can we wrap this instead e.g. from:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur, :cellar => :any_skip_relocation

to:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur,
       cellar: :any_skip_relocation

Can we wrap this instead e.g. from:

Or, to keep things more greppable keep the exception but output:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

or, if it's not too late to mess around with the format a bit further (as once we start writing this we can't go back really):
sha256 tag: :big_sur, cellar: :any_skip_relocation, value: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

or something, I dunno.

Can we wrap this instead e.g. from:

Or, to keep things more greppable keep the exception but output:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

or, if it's not too late to mess around with the format a bit further (as once we start writing this we can't go back really):
sha256 tag: :big_sur, cellar: :any_skip_relocation, value: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

or something, I dunno.

I think being able to grep it is nice, so I would rather keep it on one line. I also like it on one line, it looks nicer to my eye.
Your proposals:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

This one has the drawback that it might look weird with multiple tags:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :catalina
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur
sha256 cellar: :any, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :linux

sha256 tag: :big_sur, cellar: :any_skip_relocation, value: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"
I think this might have simplified some code, but only by a few lines here and there. It also means that I need to do yet another PR to read this new format, and let that be pushed to our users with release 3.0.0 first. It's also slightly longer.
From a "legacy syntax" point of view I find that the current implementation that was chosen is the best, as it does not change the DSL that much, it just moves the cellar information to the sha256 line, so it will probably not confuse the maintainer eye that much.

I think being able to grep it is nice, so I would rather keep it on one line. I also like it on one line, it looks nicer to my eye.
Your proposals:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

This one has the drawback that it might look weird with multiple tags:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :catalina
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur
sha256 cellar: :any, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :linux

sha256 tag: :big_sur, cellar: :any_skip_relocation, value: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"
I think this might have simplified some code, but only by a few lines here and there. It also means that I need to do yet another PR to read this new format, and let that be pushed to our users with release 3.0.0 first. It's also slightly longer.
From a "legacy syntax" point of view I find that the current implementation that was chosen is the best, as it does not change the DSL that much, it just moves the cellar information to the sha256 line, so it will probably not confuse the maintainer eye that much.

This one has the drawback that it might look weird with multiple tags:

True.

From a "legacy syntax" point of view I find that the current implementation that was chosen is the best, as it does not change the DSL that much, it just moves the cellar information to the sha256 line, so it will probably not confuse the maintainer eye that much.

Yeh, my main issue (that I apologise for not considering before now) is that unless your editor has line wrapping enabled (notably: the GitHub diff UI does not): the cellar information no longer visible without scrolling. I don't think this is the end of the world but it is a cause for concern.

I think this might have simplified some code, but only by a few lines here and there. It also means that I need to do yet another PR to read this new format, and let that be pushed to our users with release 3.0.0 first. It's also slightly longer.

I'd rather hold off on 3.0.0 and get this syntax to be one that we're 100% happy with rather than rush and be stuck with something non-ideal either forever or have to make a similar disruptive change in the medium term. @Homebrew/maintainers what do you think about the above? What's your preferred syntax (of anything suggested or come up with your own version).

This one has the drawback that it might look weird with multiple tags:

True.

From a "legacy syntax" point of view I find that the current implementation that was chosen is the best, as it does not change the DSL that much, it just moves the cellar information to the sha256 line, so it will probably not confuse the maintainer eye that much.

Yeh, my main issue (that I apologise for not considering before now) is that unless your editor has line wrapping enabled (notably: the GitHub diff UI does not): the cellar information no longer visible without scrolling. I don't think this is the end of the world but it is a cause for concern.

I think this might have simplified some code, but only by a few lines here and there. It also means that I need to do yet another PR to read this new format, and let that be pushed to our users with release 3.0.0 first. It's also slightly longer.

I'd rather hold off on 3.0.0 and get this syntax to be one that we're 100% happy with rather than rush and be stuck with something non-ideal either forever or have to make a similar disruptive change in the medium term. @Homebrew/maintainers what do you think about the above? What's your preferred syntax (of anything suggested or come up with your own version).

Maybe something simple like:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f", :big_sur, :any_skip_relocation

I think it's descriptive enough.
So the last argument is always cellar and it's default value could be nil or HOMEBREW_CELLAR or something else.

Maybe something simple like:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f", :big_sur, :any_skip_relocation

I think it's descriptive enough.
So the last argument is always cellar and it's default value could be nil or HOMEBREW_CELLAR or something else.

sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f", :big_sur, :any_skip_relocation

Is this valid ruby syntax? We must not forget that the part after the sha256 needs to be a hash with key/value pairs.

sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f", :big_sur, :any_skip_relocation

Is this valid ruby syntax? We must not forget that the part after the sha256 needs to be a hash with key/value pairs.

Is this valid ruby syntax?

Yes.

We must not forget that the part after the sha256 needs to be a hash with key/value pairs.

With the current implementation yes but technically: shouldn't need to be. We could potentially even do something weird like:
big_sur cellar: :any_skip_relocation, sha256: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

Is this valid ruby syntax?

Yes.

We must not forget that the part after the sha256 needs to be a hash with key/value pairs.

With the current implementation yes but technically: shouldn't need to be. We could potentially even do something weird like:
big_sur cellar: :any_skip_relocation, sha256: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

I'm partial to anything that puts the actual sha256 value to the right since it's not really meant for human consumption. I think the keys and values for the sha256 hash in the bottle block are the wrong way around...
That said, I've also grown accustomed to looking at the right in the bottle block, and that might be a hard habit to break.

I'm partial to anything that puts the actual sha256 value to the right since it's not really meant for human consumption. I think the keys and values for the sha256 hash in the bottle block are the wrong way around...
That said, I've also grown accustomed to looking at the right in the bottle block, and that might be a hard habit to break.

If we say that we keep the sha256 on the left, I think this excludes the following one:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

because from a reader's point of view, it feels weird that it starts with sha256, but the sha256 hexdigest is way further to the right. Also, the drawback of this syntax is that for multiple tags, it might look weird:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :catalina
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur
sha256 cellar: :any, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :linux

This leads me to think that the remaining options are (if we do not want to change too many things):
1
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur, cellar: :any_skip_relocation

2
sha256 :big_sur => "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f", cellar: :any_skip_relocation

Option 1 is the one I propose here and what has been implemented. Option 2 is doable but requires some more changes.
Option 3 is to go more exotic and make more changes, like introducing new "tag" and/or "hexdigest" keys.

If we say that we keep the sha256 on the left, I think this excludes the following one:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

because from a reader's point of view, it feels weird that it starts with sha256, but the sha256 hexdigest is way further to the right. Also, the drawback of this syntax is that for multiple tags, it might look weird:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :catalina
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur
sha256 cellar: :any, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :linux

This leads me to think that the remaining options are (if we do not want to change too many things):
1
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur, cellar: :any_skip_relocation

2
sha256 :big_sur => "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f", cellar: :any_skip_relocation

Option 1 is the one I propose here and what has been implemented. Option 2 is doable but requires some more changes.
Option 3 is to go more exotic and make more changes, like introducing new "tag" and/or "hexdigest" keys.

I actually like Mike's suggestion above:
big_sur cellar: :any_skip_relocation, sha256: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

It's nice, readable, and puts the most valuable information first.
Of course, this probably requires a whole lot more work...

I actually like Mike's suggestion above:
big_sur cellar: :any_skip_relocation, sha256: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

It's nice, readable, and puts the most valuable information first.
Of course, this probably requires a whole lot more work...

this probably requires a whole lot more work.

Yes, it does. I know that part of the code now, so I know what needs to be done for this, and with the hindsight now from my previous experience, it will take some time.

I opened an issue about the syntax and my plans on the 26th of November, so roughly 2 months ago, and nobody commented on the syntax change (#9315)
We shipped the reading part of the new syntax 1 week ago without bigger discussion
This PR was ready and approved, and the discussion just started because I disabled the "line to long" check

So it's a little bit sad to me that I wasted so much time implementing this without much more comments on the syntax, and then we decided to change it just a few hours before I wanted to pull the trigger :(

this probably requires a whole lot more work.

Yes, it does. I know that part of the code now, so I know what needs to be done for this, and with the hindsight now from my previous experience, it will take some time.

I opened an issue about the syntax and my plans on the 26th of November, so roughly 2 months ago, and nobody commented on the syntax change (#9315)
We shipped the reading part of the new syntax 1 week ago without bigger discussion
This PR was ready and approved, and the discussion just started because I disabled the "line to long" check

So it's a little bit sad to me that I wasted so much time implementing this without much more comments on the syntax, and then we decided to change it just a few hours before I wanted to pull the trigger :(

So it's a little bit sad to me that I wasted so much time implementing this

I'm aware, and I feel a bit guilty even sharing my thoughts about it. However, thinking about it more, I think the improvements from a major overhaul might be too marginal to be worth the investment.
Between your option 1 and 2, though, I prefer option 1, as it mirrors more closely how sha256 lines look elsewhere (and I think that makes things more readable).

So it's a little bit sad to me that I wasted so much time implementing this

I'm aware, and I feel a bit guilty even sharing my thoughts about it. However, thinking about it more, I think the improvements from a major overhaul might be too marginal to be worth the investment.
Between your option 1 and 2, though, I prefer option 1, as it mirrors more closely how sha256 lines look elsewhere (and I think that makes things more readable).

and the discussion just started because I disabled the "line to long" check

I'm also sad about this and I apologise for not catching this until now. I think the problem is that I only just noticed that that "line too long" check was going to kick in for multiple lines on every single formula with a bottle and that we're going to be putting important information on the end. I debated whether or not to say anything and I figured: better a bit more pain now (which I can try to be more helpful with) than regretting our decision. I'll happily be overruled on that, though, as I think reordering could achieve some of the same result.

and the discussion just started because I disabled the "line to long" check

I'm also sad about this and I apologise for not catching this until now. I think the problem is that I only just noticed that that "line too long" check was going to kick in for multiple lines on every single formula with a bottle and that we're going to be putting important information on the end. I debated whether or not to say anything and I figured: better a bit more pain now (which I can try to be more helpful with) than regretting our decision. I'll happily be overruled on that, though, as I think reordering could achieve some of the same result.

I think reordering like this:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

doest not need a big change, I just need to change the order of writing, and the reading should be fine. As it's a hash and we made sure to not depend on the order (you made me remove all the .first calls, remember? :D), it should work.
I just find it a little bit ugly as the cellar information might be missing on some lines, and the hexdigests will not be aligned anymore.

I think reordering like this:
sha256 cellar: :any_skip_relocation, "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur

doest not need a big change, I just need to change the order of writing, and the reading should be fine. As it's a hash and we made sure to not depend on the order (you made me remove all the .first calls, remember? :D), it should work.
I just find it a little bit ugly as the cellar information might be missing on some lines, and the hexdigests will not be aligned anymore.

I think the keys and values for the sha256 hash in the bottle block are the wrong way around...

I've always thought this as well.
I would suggest
``ruby
sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

and please accept my apologies for the late stage bike shedding, Michka, but if we've opened this Pandora's box, that's my two cents.

I think the keys and values for the sha256 hash in the bottle block are the wrong way around...

I've always thought this as well.
I would suggest
``ruby
sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

and please accept my apologies for the late stage bike shedding, Michka, but if we've opened this Pandora's box, that's my two cents.

I just find it a little bit ugly as the cellar information might be missing on some lines, and the hexdigests will not be aligned anymore.

Agreed.

I just find it a little bit ugly as the cellar information might be missing on some lines, and the hexdigests will not be aligned anymore.

Agreed.

This feels a bit wrong to suggest—especially after already adding read support for the new syntax—but what about changing cellar to accept a hash?
For example:
bottle do
  cellar big_sur:  :any_skip_relocation,
         catalina: :any_skip_relocation
  sha256 "1a9fd37585d31ef6f1c61b431bfab4fde9755db5ee081d8eba1c9a358278481f" => :big_sur
  sha256 "99aea0397bae82f956eab6868704b493b99f9160e4c5f8f558937db3e349b96b" => :catalina
end

or:
bottle do
  cellar big_sur: :any_skip_relocation
  cellar catalina: :any_skip_relocation
  sha256 "1a9fd37585d31ef6f1c61b431bfab4fde9755db5ee081d8eba1c9a358278481f" => :big_sur
  sha256 "99aea0397bae82f956eab6868704b493b99f9160e4c5f8f558937db3e349b96b" => :catalina
end

This feels a bit wrong to suggest—especially after already adding read support for the new syntax—but what about changing cellar to accept a hash?
For example:
bottle do
  cellar big_sur:  :any_skip_relocation,
         catalina: :any_skip_relocation
  sha256 "1a9fd37585d31ef6f1c61b431bfab4fde9755db5ee081d8eba1c9a358278481f" => :big_sur
  sha256 "99aea0397bae82f956eab6868704b493b99f9160e4c5f8f558937db3e349b96b" => :catalina
end

or:
bottle do
  cellar big_sur: :any_skip_relocation
  cellar catalina: :any_skip_relocation
  sha256 "1a9fd37585d31ef6f1c61b431bfab4fde9755db5ee081d8eba1c9a358278481f" => :big_sur
  sha256 "99aea0397bae82f956eab6868704b493b99f9160e4c5f8f558937db3e349b96b" => :catalina
end

Should we move this discussion to #9315?

Let's finish it here.
@SeekingMeaning  regarding your proposal:
bottle do
  cellar big_sur: :any_skip_relocation
  cellar catalina: :any_skip_relocation
  sha256 "1a9fd37585d31ef6f1c61b431bfab4fde9755db5ee081d8eba1c9a358278481f" => :big_sur
  sha256 "99aea0397bae82f956eab6868704b493b99f9160e4c5f8f558937db3e349b96b" => :catalina
end

this means reimplementing the whole collector logic for these lines too. There is a bunch of logic around the order of the tags in bottle blocks: I think this change would be bigger from a code point of view (at least from what I remember dealing with last time).

Should we move this discussion to #9315?

Let's finish it here.
@SeekingMeaning  regarding your proposal:
bottle do
  cellar big_sur: :any_skip_relocation
  cellar catalina: :any_skip_relocation
  sha256 "1a9fd37585d31ef6f1c61b431bfab4fde9755db5ee081d8eba1c9a358278481f" => :big_sur
  sha256 "99aea0397bae82f956eab6868704b493b99f9160e4c5f8f558937db3e349b96b" => :catalina
end

this means reimplementing the whole collector logic for these lines too. There is a bunch of logic around the order of the tags in bottle blocks: I think this change would be bigger from a code point of view (at least from what I remember dealing with last time).

The same holds for the proposal for @MikeMcQuaid and @carlocab :
big_sur cellar: :any_skip_relocation, sha256: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

This means creating a "big_sur" function on the fly, and a bunch of functions for each platform: the whole collector logic in the code will be impacted, and this means creating tag specific functions for ALL possible platforms. From my point of view, this change in the code will be massive.

The same holds for the proposal for @MikeMcQuaid and @carlocab :
big_sur cellar: :any_skip_relocation, sha256: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

This means creating a "big_sur" function on the fly, and a bunch of functions for each platform: the whole collector logic in the code will be impacted, and this means creating tag specific functions for ALL possible platforms. From my point of view, this change in the code will be massive.

I'm happy with option 1:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur, :cellar => :any_skip_relocation

This preserves the uniformity of appearance with the sha256 lines elsewhere and is already readable for anyone who currently looks inside the bottle blocks. One will have to look to the end of the line for the useful information currently anyway, and this is an improvement on that by including more useful information where you're already looking.
It would've been nice to put all the interesting information first, but I don't mind chalking this up to something that one would do were they starting from scratch.

I'm happy with option 1:
sha256 "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f" => :big_sur, :cellar => :any_skip_relocation

This preserves the uniformity of appearance with the sha256 lines elsewhere and is already readable for anyone who currently looks inside the bottle blocks. One will have to look to the end of the line for the useful information currently anyway, and this is an improvement on that by including more useful information where you're already looking.
It would've been nice to put all the interesting information first, but I don't mind chalking this up to something that one would do were they starting from scratch.

This feels a bit wrong to suggest—especially after already adding read support for the new syntax, and this PR nearly being merged—but what about changing #cellar to accept a hash?

I think it's good to keep together the information for each platform on a line for each platform.

This means creating a "big_sur" function on the fly, and a bunch of functions for each platform: the whole collector logic in the code will be impacted, and this means creating tag specific functions for ALL possible platforms. From my point of view, this change in the code will be massive.

It's actually relatively easy to do this in Ruby (and I'm happy to be the one to do so, to be clear). Let's focus on what we would ideally have it look like and ignore the implementation for now.

One has to look to the end of the line for the useful information at the moment anyway, and this is an improvement on that by including more useful information where you're already looking.

The big difference is: this information will be hidden in e.g. the GitHub diff UI without scrolling now.

It would've been nice to put all the interesting information first, but I don't mind chalking this up as something that one would do were they starting (everything, i.e. brew with binary bottles from the ground up) from scratch.

I mean: we are kinda redoing the bottle lines from scratch?

I would suggest
sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

This makes sense to me, too. I'd be fine with the line being longer if this is the case because the training information is the least important.
If I do have to factor in "what's the minimal changes required to make this happen": this is probably my preference. @iMichka what do you think? If you're happy enough with that, I should be able to open a PR today to make this syntax work.

This feels a bit wrong to suggest—especially after already adding read support for the new syntax, and this PR nearly being merged—but what about changing #cellar to accept a hash?

I think it's good to keep together the information for each platform on a line for each platform.

This means creating a "big_sur" function on the fly, and a bunch of functions for each platform: the whole collector logic in the code will be impacted, and this means creating tag specific functions for ALL possible platforms. From my point of view, this change in the code will be massive.

It's actually relatively easy to do this in Ruby (and I'm happy to be the one to do so, to be clear). Let's focus on what we would ideally have it look like and ignore the implementation for now.

One has to look to the end of the line for the useful information at the moment anyway, and this is an improvement on that by including more useful information where you're already looking.

The big difference is: this information will be hidden in e.g. the GitHub diff UI without scrolling now.

It would've been nice to put all the interesting information first, but I don't mind chalking this up as something that one would do were they starting (everything, i.e. brew with binary bottles from the ground up) from scratch.

I mean: we are kinda redoing the bottle lines from scratch?

I would suggest
sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

This makes sense to me, too. I'd be fine with the line being longer if this is the case because the training information is the least important.
If I do have to factor in "what's the minimal changes required to make this happen": this is probably my preference. @iMichka what do you think? If you're happy enough with that, I should be able to open a PR today to make this syntax work.

I'm fine with that (e.g.)
sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

I wont have time to open a PR for this right now, maybe way later today, so if you can beat me to it, I'll let you do it. That PR should only change the reading part to be able to read the new syntax. Once this is done and merged, I'll rebase this PR accordingly.

I'm fine with that (e.g.)
sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

I wont have time to open a PR for this right now, maybe way later today, so if you can beat me to it, I'll let you do it. That PR should only change the reading part to be able to read the new syntax. Once this is done and merged, I'll rebase this PR accordingly.

sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

This would make the bottle block much more difficult to read in the (admittedly rather unlikely) event that different versions have different cellar values.

sha256 cellar: :any_skip_relocation, big_sur: "a0af7dcbb5c83f6f3f7ecd507c2d352c1a018f894d51ad241ce8492fa598010f"

This would make the bottle block much more difficult to read in the (admittedly rather unlikely) event that different versions have different cellar values.

Both solutions have drawbacks:

Putting the cellar information on the left makes the hexdigest non-aligned and less readable when different cellar information is used based on the platform (my concerns above)
Putting it on the right forces you to scroll to see that information if you have a small window, or in the github UI diff.

So, you need to chose which one is less worse.

Both solutions have drawbacks:

Putting the cellar information on the left makes the hexdigest non-aligned and less readable when different cellar information is used based on the platform (my concerns above)
Putting it on the right forces you to scroll to see that information if you have a small window, or in the github UI diff.

So, you need to chose which one is less worse.

Putting it on the right forces you to scroll to see that information if you have a small window, or in the github UI diff.

I use a screen on the smaller side (13"), and I usually have enough window space to see the whole sha256 line in the cellar block with lots of room left over. I'm also partial to unified diffs. And I think the split diffs wrap. If so, then it may not be a matter of scrolling as an issue of reading wrapped lines.
Anyway, you're right about both having drawbacks. I don't mind trusting your judgment on this.

Putting it on the right forces you to scroll to see that information if you have a small window, or in the github UI diff.

I use a screen on the smaller side (13"), and I usually have enough window space to see the whole sha256 line in the cellar block with lots of room left over. I'm also partial to unified diffs. And I think the split diffs wrap. If so, then it may not be a matter of scrolling as an issue of reading wrapped lines.
Anyway, you're right about both having drawbacks. I don't mind trusting your judgment on this.

I wont have time to open a PR for this right now, maybe way later today, so if you can beat me to it, I'll let you do it. That PR should only change the reading part to be able to read the new syntax. Once this is done and merged, I'll rebase this PR accordingly.

Working on it now 👍🏻

I wont have time to open a PR for this right now, maybe way later today, so if you can beat me to it, I'll let you do it. That PR should only change the reading part to be able to read the new syntax. Once this is done and merged, I'll rebase this PR accordingly.

Working on it now 👍🏻

Once this is done and merged, I'll rebase this PR accordingly.

Have done this for you too.

Once this is done and merged, I'll rebase this PR accordingly.

Have done this for you too.

Thanks. I think you messed something up with the branches here :D

Thanks. I think you messed something up with the branches here :D

Oops can you try to reopen?

Oops can you try to reopen?

Try to change the base branch back to master.

Try to change the base branch back to master.

Try to change the base branch back to master.

How do I do that. In git?
I can repush my local branch without your last commit, is that ok for you?

Try to change the base branch back to master.

How do I do that. In git?
I can repush my local branch without your last commit, is that ok for you?

Superseded by PR https://github.com/Homebrew/brew/pull/10449

Superseded by PR https://github.com/Homebrew/brew/pull/10449


Make versioned binaries available

I became interested in this tool but I'd have to setup lots of things in-house like CICD for building it, some container registry for storing it, and Kubernetes manifests to manage the installation in my cluster.
I'd like to contribute such features myself and I'd appreciate some guidance to do it.

Hey @7onn, would you be open to instantiate this exporter via Grafana Agent? If so, this is all available for you. https://grafana.com/docs/agent/latest/flow/reference/components/prometheus.exporter.snowflake/

That is awesome! And provided I can remotely write on my Prometheus, I can use the Alertmanager (our state of art) to manage the alerts. We'll make some tests with it.


Writing to frag_color multiple times in shader seems to cause performance problem

Godot version
3.6 beta 3 (but likely existed for years)
System information
Linux Mint 21.1, Intel Core i7-13700T, Intel a780 GPU
Issue description
After spending some time tracking down a performance problem when working on core shaders in the 3D platformer demo, I tracked down a large drop in fill rate performance to modifying frag_color multiple times in the fragment shader.
I'm half expecting this to be an error on my part but I'd rather flag it and feel silly :blush: than let it pass and potentially miss a good speedup.
GLES2
Adding the second line here:
gl_FragColor = vec4(ambient_light + diffuse_light + specular_light, alpha);
gl_FragColor *= 1.0001;

dropped the frame rate from 127fps to 62fps in GLES2.
Whereas using this instead:
vec4 temp_frag_color = vec4(ambient_light + diffuse_light + specular_light, alpha);
temp_frag_color *= 1.0001;
gl_FragColor = temp_frag_color;

kept the frame rate at 127fps.
This was very surprising to me, and I was expecting it to be an artifact, but it seems very repeatable on my hardware. It may come down to the drivers, but if it happens for me it likely happens on other setups.
This paradigm of writing to frag_color multiple times is used in GLES2 and GLES3 (for at least fog and emission), and may be dropping performance unnecessarily. It may also be a problem in 4.x (haven't examined but I've mentioned this in rocket chat).
GLES3
I also tried the same in GLES3.
Again, adding the second line here:
	frag_color = vec4(ambient_light + diffuse_light + specular_light, alpha);
	frag_color.rgb *= 1.0001;

dropped performance from 88fps to 30fps. That's almost a 3 fold drop in performance.
GLES3 seems less susceptible currently, as it only modifies frag_color when adding emission:
#ifdef USE_FORWARD_LIGHTING //ubershader-runtime
	frag_color.rgb += emission;
#endif //ubershader-runtime

Steps to reproduce
See above.
Minimal reproduction project
3D Platformer demo, modify scene.glsl shaders as above, and recompile engine.
Discussion
I'm not a shader guru by any means. I had a vague memory of this as a possible thing to watch for, but I'm not up to date. I haven't previously touched this part of the shaders, I just noticed while running experiments with blob shadows.
It could be that modifying the existing frag_color is causing some kind of round trip, or preventing this being optimized away into a local register, whereas using a temporary variable prevents side effects so doesn't have the performance drop. Alternatively it could be a problem introduced by our own shader "translator" code (I haven't examined the final glsl).
I also considered the problem was due to modification of alpha, but using a line like gl_FragColor.rgb *= 0.999; also creates the problem.
I'm still not absolutely sure it isn't some artifact I've created somehow - it would be nice to see it independently verified. It may not occur on all hardware / drivers.
It is possible this could also occur in user shaders.
If confirmed perhaps we can take some (hopefully) simple steps to eliminate this problem, by standardizing our shaders to use temporaries, and only write to the final GL builtin once, at the end.
Update
My small PR for GLES2 would seem to confirm this is a valid issue, it gives 2-3x increase in fps in scenes which tax fill rate and use emission / fog. :+1:
It is likely that the 3D platformer is taxing on fill rate perhaps because of reflection probes. Will try this in TPS demo too.

Intel a780 GPU

That GPU was never released, are you mistaking it for the Arc A770 or A380?

Intel a780 GPU

That GPU was never released, are you mistaking it for the Arc A770 or A380?

That's what I read too, but my system info says "Intel Corporation Device a780" :rofl:
Anyway the CPU is 13th Gen Intel© Core™ i7-13700T × 16, so it is whatever integrated GPU comes with that. :+1:

Does this come into play when doing this in user created shaders:
COLOR = vec4(1.0,0.0,0.0,1.0)
COLOR = vec4(0.0,1.0,0.0,1.0)

Does this get transpiled to multiple assigns to gl_fragColor?
Any idea on webgl implication?
Is this a super easy fix? Just change a color .glsl files?

Does this come into play when doing this in user created shaders:

Possibly, I'm not super familiar with the translation yet. But on the plus side it should be relatively easy to fix (write to temporary, then write gl_FragColor once at the end).
I've not tested outside my dev machine yet, but in theory if it occurs on one machine, it's likely it could occur on multiple similar setups. (Only further testing will reveal this.)

Further clue this morning - while testing #84529 I discovered that in GLES3 adding this second line:
frag_color_final = frag_color;
frag_color_final *= 0.999;

Does NOT result in the drop in performance. This suggests that there is some kind of interaction with the previous code, like it is breaking a fast path, but only in some circumstances depending on the previous code.
Truly a very strange bug. :grin:

This is not perfect but if you run this project in 3.x it should hopefully show whether your GPU has this slowdown (in GLES2). This may not be reliable on super fast GPU, it shows approx 100fps on my integrated GPU.
If the FPS label is approx the same with fog on and off, there is no slowdown. If it is approx halved with fog on, then your GPU has the slowdown.
glFragColor_test_gpu.zip

Will post results here of testing as I get it. Using the above project.
Linux Mint 21.1, Intel Core i7-13700T, Intel UHD Graphics 770 GPU
Fog on 46fps
Fog off 104fps
Very susceptible.
Linux Mint 21.2, Intel Core i3 2377M, 2nd Gen Core integrated graphics
Fog on 16fps
Fog off 60fps
Very susceptible.

This is fixed in 3.x with #84529 but the problem may still occur in 4.x (where my PR is now out of date #83697 ).
Changing milestone to 4.x to represent this, and let anyone else pick this up if they get the problem in 4.x.


bug: no specs found for module prisma

Did you check docs and existing issues?

[X] I have read all the LazyVim docs
[X] I have searched the existing issues of LazyVim
[X] I have searched the existing issues of plugins related to this issue

Neovim version (nvim -v)
v0.9.5
Operating system/version
MacOS 14.2.1
Describe the bug
Trying to install Prisma by adding the extras line for it:
{ import = "lazyvim.plugins.extras.lang.prisma" }

However, I just get this error message:
No specs found for module lazyvim.plugins.extras.lang.prisma

Steps To Reproduce

Add prisma import
Restart nvim

Expected Behavior
It should install appropriate Prisma packages
Repro
-- DO NOT change the paths and don't remove the colorscheme
local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({ "git", "clone", "--filter=blob:none", "https://github.com/folke/lazy.nvim.git", lazypath, })
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  "folke/tokyonight.nvim",
  { "LazyVim/LazyVim", import = "lazyvim.plugins" },
  { import = "lazyvim.plugins.extras.lang.prisma" }
  -- add any other plugins here
}
require("lazy").setup(plugins, {
  root = root .. "/plugins",
})

vim.cmd.colorscheme("tokyonight")
-- add anything else here

Are you sure LazyVim is updated to latest version?

An update did solve the issue. Sorry about that and thanks.


Add instructions for adding the GraphQL plugin

In order for the SNARK worker dashboard to work you need the GraphQL installed and configured as a datasource https://grafana.com/grafana/plugins/fifemon-graphql-datasource

Yes right. thank you for telling me.
It is actually developed with the mentioned plug-in, but explanation is omitted in the README.md
Task

Add plug-in installation method to REAMD.md


feat: implement streaming support and output formats

Feature: Implement Streaming Support and Output Formats
This PR implements high-priority features including streaming support and custom output formats, along with comprehensive test coverage.
Changes

Added support for JSON, XML, and CSV output formats with schema validation
Implemented streaming with experimental_stream
Added comprehensive test coverage for all output formats
Fixed TypeScript errors with proper type definitions
Updated dependencies for AI SDK packages

Testing

Added tests for all output formats (JSON, XML, CSV)
Added streaming tests with different scenarios
Verified error handling for invalid formats
All tests passing locally

Documentation

Updated TODO.md to reflect completed tasks
Added type definitions for new features

Dependencies
Added required type definitions:

@ai-sdk/provider
@ai-sdk/provider-utils
@ai-sdk/ui-utils
@opentelemetry/api

Link to Devin run: https://app.devin.ai/sessions/0cf889ecb8ba4539995bca73de6b8353

we do not need output formats in the library - that was only intended for the CLI, and that is low priority.  Remove output formats from the API interface.


PrintNow shows only Grayscale but the printer is able to print with color.

this is based on the issue https://gitlab.com/cortado-group/thinprint/ezeep-dash/ezd-frontend/-/issues/30


non wasm chains always seem to fail on build w/ standard simapp

if wasmvm is not found in go.mod or go.sum, I constantly get wasm errors. But not for anything using wasm with the local docker.  If I just add wasmvm 1.5.2 to the go.mod/go.sum (without any app usage) it builds fine since it can download.
- name: minimalpoa
  dockerfile: cosmos
  build-target: make install
  binaries:
    - /go/bin/aaaaad
  build-env:
    - LEDGER_ENABLED=false

It's possible it just needs a build-env or something. But feel this was meant to be automatic
GH Action logs
#11 [go-builder 3/7] WORKDIR /code
#11 DONE 0.0s
#12 [go-builder 4/7] ADD go.mod go.sum ./
#12 DONE 0.0s
#13 [go-builder 5/7] RUN set -eux;     export ARCH=$(uname -m);     WASM_VERSION=$(go list -m all | grep github.com/CosmWasm/wasmvm | awk '{print $2}');     if [ ! -z "${WASM_VERSION}" ]; then       wget -O /lib/libwasmvm_muslc.a [https://github.com/CosmWasm/wasmvm/releases/download/${WASM_VERSION}/libwasmvm_muslc.${ARCH}.a;](https://github.com/CosmWasm/wasmvm/releases/download/$%7BWASM_VERSION%7D/libwasmvm_muslc.$%7BARCH%7D.a;)     fi;     go mod download;
#13 0.051 + set -eux
#13 0.051 + uname -m
#13 0.052 + export 'ARCH=x86_64'
#13 0.053 + go list -m all
#13 0.055 + grep github.com/CosmWasm/wasmvm
#13 0.057 + awk '{print $2}'
#13 8.725 + WASM_VERSION=
#13 ERROR: process "/bin/sh -ecuxo pipefail set -eux;     export ARCH=$(uname -m);     WASM_VERSION=$(go list -m all | grep github.com/CosmWasm/wasmvm | awk '{print $2}');     if [ ! -z \"${WASM_VERSION}\" ]; then       wget -O /lib/libwasmvm_muslc.a [https://github.com/CosmWasm/wasmvm/releases/download/${WASM_VERSION}/libwasmvm_muslc.${ARCH}.a;](https://github.com/CosmWasm/wasmvm/releases/download/$%7BWASM_VERSION%7D/libwasmvm_muslc.$%7BARCH%7D.a;)     fi;     go mod download;" did not complete successfully: exit code: 1
------
 > [go-builder 5/7] RUN set -eux;     export ARCH=$(uname -m);     WASM_VERSION=$(go list -m all | grep github.com/CosmWasm/wasmvm | awk '{print $2}');     if [ ! -z "${WASM_VERSION}" ]; then       wget -O /lib/libwasmvm_muslc.a [https://github.com/CosmWasm/wasmvm/releases/download/${WASM_VERSION}/libwasmvm_muslc.${ARCH}.a;](https://github.com/CosmWasm/wasmvm/releases/download/$%7BWASM_VERSION%7D/libwasmvm_muslc.$%7BARCH%7D.a;)     fi;     go mod download;:
0.051 + set -eux
0.051 + uname -m
0.052 + export 'ARCH=x86_64'
0.053 + go list -m all
0.055 + grep github.com/CosmWasm/wasmvm
0.057 + awk '{print $2}'
------
Dockerfile:10
--------------------
   9 |     ADD go.mod go.sum ./
  10 | >>> RUN set -eux; \
  11 | >>>     export ARCH=$(uname -m); \
  12 | >>>     WASM_VERSION=$(go list -m all | grep github.com/CosmWasm/wasmvm | awk '{print $2}'); \
  13 | >>>     if [ ! -z "${WASM_VERSION}" ]; then \
  14 | >>>       wget -O /lib/libwasmvm_muslc.a [https://github.com/CosmWasm/wasmvm/releases/download/${WASM_VERSION}/libwasmvm_muslc.${ARCH}.a;](https://github.com/CosmWasm/wasmvm/releases/download/$%7BWASM_VERSION%7D/libwasmvm_muslc.$%7BARCH%7D.a;) \
  15 | >>>     fi; \
  16 | >>>     go mod download;
  17 |
--------------------
ERROR: failed to solve: process "/bin/sh -ecuxo pipefail set -eux;     export ARCH=$(uname -m);     WASM_VERSION=$(go list -m all | grep github.com/CosmWasm/wasmvm | awk '{print $2}');     if [ ! -z \"${WASM_VERSION}\" ]; then       wget -O /lib/libwasmvm_muslc.a [https://github.com/CosmWasm/wasmvm/releases/download/${WASM_VERSION}/libwasmvm_muslc.${ARCH}.a;](https://github.com/CosmWasm/wasmvm/releases/download/$%7BWASM_VERSION%7D/libwasmvm_muslc.$%7BARCH%7D.a;)     fi;     go mod download;" did not complete successfully: exit code: 1
Error: buildx failed with: ERROR: failed to solve: process "/bin/sh -ecuxo pipefail set -eux;     export ARCH=$(uname -m);     WASM_VERSION=$(go list -m all | grep github.com/CosmWasm/wasmvm | awk '{print $2}');     if [ ! -z \"${WASM_VERSION}\" ]; then       wget -O /lib/libwasmvm_muslc.a [https://github.com/CosmWasm/wasmvm/releases/download/${WASM_VERSION}/libwasmvm_muslc.${ARCH}.a;](https://github.com/CosmWasm/wasmvm/releases/download/$%7BWASM_VERSION%7D/libwasmvm_muslc.$%7BARCH%7D.a;)     fi;     go mod download;" did not complete successfully: exit code: 1

nope nvm this is my own docker file. I was not using a heighliner action instance, was using buildx


add treeshaking to build process

figure out how to add add a treeshaking algorithm to the build process for smaller file sizes.
@rollup is looking promising as a replacement for browserify. unfortunately the online guide is currently missing some info on how to use it with commonjs modules from npm (like node-canvas).

fixed with e59dd64 in 0.5.1.


python3Packages.confuse: 1.4.0 -> 1.5.0

Motivation for this change
Update to latest upstream release 1.5.0
Things done

[x] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)

[x] NixOS
[ ] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
[ ] Tested execution of all binary files (usually in ./result/bin/)
21.11 Release Notes (or backporting 21.05 Relase notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[x] Fits CONTRIBUTING.md.

Result of nixpkgs-review pr 129090 run on x86_64-linux 1

6 packages built:

beets
beetsExternalPlugins.alternatives
beetsExternalPlugins.copyartifacts
beetsExternalPlugins.extrafiles
python38Packages.confuse
python39Packages.confuse

Result of nixpkgs-review pr 129090 at e329096f run on aarch64-linux 1

6 packages built successfully:

beets
beetsExternalPlugins.alternatives
beetsExternalPlugins.copyartifacts
beetsExternalPlugins.extrafiles
python38Packages.confuse
python39Packages.confuse


Immutable DB: Do "blocks in future" check after ChainDB almost initialized

At the moment, the imm DB verifies that there are no blocks from the future as it walks over the chunk files. However, the translation from SlotNo to UTCTime is ledger dependent :roll_eyes: We should instead initialize the imm DB and ledger DB first, not checking time stamps, and only then check the tip of the imm DB. If that is ahead of the current time, we should trim the imm DB and kill the node.

Marking as high priority since this is blocking the hard fork combinator.

This is a bit subtle. After discussing this with @mrBliss , we came to the following conclusions:

For any chain, we can for sure translate any of the SlotNo of the blocks within that chain to the corresponding UTCTime (given the ledger state corresponding to the tip of that chain).
This means that we can translate the SlotNo at the tip of the chain to a UTCTime
We can then compare that UTCTime to the current UTCTime to check if that block is in the future.
We also have the property that if we can translate a time t to a SlotNo, we can also convert any time t' < t to a SlotNo
We can take advantage of (4) to translate the current time to a SlotNo if the tip of the chain is indeed in the future.
That SlotNo can then be used either as an optional bound to the parser (re-validate) or so a new deleteFromSlot API call.

Ok, change in plans here, although the exact plan is not yet clear in my head :) But basically, we should avoid basing this on time.

We disabled the check for now in #1908, unblocking the hard fork combinator, so bumping down the medium priority.


Minecraft.pricing

Traceback (most recent call last):
File "E:\0Skidders\listbot\start.py", line 3, in
from bot.bot import bot
File "E:\0Skidders\listbot\bot\bot.py", line 3, in
from bot.modals.evalue import Embed
File "E:\0Skidders\listbot\bot\modals\evalue.py", line 4, in
from minecraft.pricing.data_handler import user_stats, handle_stats
ModuleNotFoundError: No module named 'minecraft.pricing'

did you augment any of the existing code?

no

Yo


Y Axis in SciChart is not linking min/max when using full extents.

This issue needs to be resolved on Perforce in the PlottingLibrary API.  Currently it just uses SciChart's built in zoom to extents function, however when the Y Axes are bound to each other, it'll use the min/max of the primary binding axis instead of the cumulative min/max of all bound axes.

I'm not sure if this needs to be addressed for task 1 specifically, however that's where it was found to be problematic due to the 4 chart binding.


[global] removed explicit font-family in css

Description
I have removed all font-family css prop usages in our components except cases when it was used to display only special symbols such as quotes.
Motivation and Context
Explicit font-family properties might break application appearance by adding multiple fonts on one page.
font-family: inherit; makes totally to sense in our components.
How has this been tested?
I have updated screenshot tests and still can't get why they have changed.
Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.

is it necessary to add changes in changelogs files?

Closing in favour of https://github.com/semrush/intergalactic/pull/514


Deprecate amenity=dancing_school

Also deprecated in the wiki and a replacement is available, see https://wiki.openstreetmap.org/wiki/Tag%3Aamenity%3Ddancing_school

Thanks for all your contributions today @westnordost 👏

A facility providing professional dancing instruction may be co-located with a dancing hall, but just as well could be completely separate depending on the audience and scope (for example providing vocational education).
Just because somebody engaged in wiki-fiddling is no reason to jump to conclusions.

Okay, so the concept is that leisure=dance is a "(general purpose) dancing hall" and amenity=dancing_school is, well, a dancing school? And the latter can be within the former?
Then let's revert this and make the dancing school a preset and somebody should clear up the wiki.

Then let's revert this and make the dancing school a preset and

Dance Hall has been a preset since 2016, see #3579 and I followed the recommendation on the wiki at the time.
Dance school has been a preset since 2018, see #4846.  The contributor followed the recommendation on the wiki at the time.
I agree that the wiki could use some cleanup, but we don't need to revert anything.  We are following the guidelines the best we can, and we don't need multiple presets for Dance Halls - it's totally fine to deprecate the amenity one.

I think this is about the name. Sure, you may get tuition at a dancing venue, but this doesn't make the dancing venue a dancing school.
For example, a car dealership shop=car may also offer repair services (service:vehicle:car_repair=yes I supposed), but that doesn't make the car dealership an automobile repair shop shop=car_repair. These two are usually quite some different things, at least are referred to differently (as I just did).
Whether this is really about that, I am not sure, because the wiki is not really clear here. But at least it would make sense this way and would be consistent.

https://github.com/openstreetmap/id-tagging-schema/issues/373 proposes revert of that PR

https://github.com/openstreetmap/id-tagging-schema/issues/1177#issuecomment-2040103619 actually reverts it, I made broken PR


Evaluate objective function without parsing

Provide a function that evaluate the objective function who may have been returned by the run function.
Maybe something like:
let (solver_status, values, objective) = solver::run(&prob).unwrap();

I'm not sure whether the objective should be unconditionally evaluated. There are use cases where it's actual value isn't important, but just the values of the variables is of interest.
Thus, I  propose to make it two step:
let (status, values) = solver.run(&problem).unwrap();
let objective = problem.eval(&values);

eval is just the name I've used and I'm open for better suggestions.
Alternatively it might be a better to wrap the return value of solver.run into it's own struct (e.g. named Solution), which then provides ways of retrieving status, values and objective.

It's a good point. Returning a Solution structure seems a good ideal. Especially if we add new features such as eval. Maybe a Solution should refer to a Problem. With this, we could call eval without giving parameters.

Create a new issue for that : #39


Add x86_64 dependency note to pip installation

Currently non x86_64 linux architectures are not supported, see #7097 for request to change this. This can lead to installation confusion, as jax will install, but jaxlib will not. For example see #12307.
If this can be more clearly phrased or explained, let me know.

Thanks! Should we add the same caveat under the GPU instructions?

For the current state I think this makes sense ... It would change soon-ish with Grace and Grace-Hopper

@jakevdp good point, I added that too.

Forgot about jetsons too 😅

Yeah, a Jetson Orin was what I was trying to install Jax on and had to build from source for. The good news is building from source was pretty easy! But this would have saved me some time debugging why pip appeared to work, but jaxlib didn't install.

Let me know if there is anything else needed from me before this is merged.
I don't seem to have permissions to re-run the Read the Docs build, but from reading the output, it seems to be just two warnings, neither related to my edits. The first was that https://docs.scipy.org/doc/scipy-1.8.1/objects.inv wasn't fetchable, which seems to have been temporary and resolved. The other was undefined label: ndimage-interpolation-modes in jax._src.scipy.ndimage.map_coordinates. If wanted I can make an empty commit to force it to re-run.

Thanks @treyra - one last thing, could you please squash the changes into a single commit? (See https://jax.readthedocs.io/en/latest/contributing.html#single-change-commits-and-pull-requests) Thanks!

Done! Thanks @jakevdp for walking me through how to contribute for the first time. Hopefully in the future I can make some more substantial contributions.

Thanks for adding this clarification! Sorry it took so long to get this merged 😀


Added information regarding superscript, subscript and footnotes

Done

Hey @swapn652
Could you please resolve the merge conflicts? While doing that, please note that the footnote example done by the other PR is already in the main. So you need to discard the 'footnote` example of yours and keep the other two. Once done, we are all set to merge this PR.

Hi @swapn652 Are you on it?

@atapas done. Please check.


[5.5] Add queue assertions for chained jobs

Add additional assertions against chained jobs.
assertChained($job, $callback = null)
assertChainedTimes($job, $times = 1)
assertNotChained($job, $callback = null)
assertNothingChained()
assertChained($job, $callback = null)
Queue::fake();

$order = factory('App\Order')->create();

FirstJob::chain([
    new SecondJob($order),
])->dispatch($order);

Queue::assertChained(SecondJob::class);
Queue::assertChained(SecondJob::class, function ($job) use ($order) {
    return $job->order->id === $order->id;
});

assertChainedTimes($job, $times = 1)
Queue::fake();

FirstJob::chain([
    new SecondJob,
    new ThirdJob,
    new ThirdJob,
])->dispatch();

Queue::assertChainedTimes(SecondJob::class);
Queue::assertChainedTimes(ThidJob::class, 2);

assertNothingChained()
Queue::fake();

$order = factory('App\Order')->create();
FirstJob::dispatch($order);

Queue::assertNothingChained();

assertNotChained($job, $callback = null)
Queue::fake();

[$order1, $order2] = factory('App\Order', 2)->create();

FirstJob::chain([
    new SecondJob($order1),
])->dispatch();

Queue::assertNotChained(FirstJob::class);
Queue::assertNotChained(SecondJob::class, function ($job) use ($order2) {
    return $job->order->id === $order2->id;
});

The syntax Queue::assertChainedTimes(SecondJob::class); looks weird. I suggest that the default value is removed to force people to say how many times they expect it to be chained.

@sisve Wrong example.You can't actually use assertChainedTimes($job, $times = 1); because it's a protected method. You can pass a numeric value as a second argument to assertChainedTimes($job, 2); instead.
It's the same as using current implementation Queue::assertPushed($job, 2);

Looks great!
On Wed, 25 Oct 2017 at 04:22 Titas Gailius<EMAIL_ADDRESS>wrote:

@sisve https://github.com/sisve Wrong example.You can't actually use assertChainedTimes($job,
$times = 1); because it's a protected method. You can pass a numeric
value as a second argument to assertChainedTimes($job, 2); instead.
It's the same as using current implementation Queue::assertPushed($job,
2);
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/laravel/framework/pull/21806#issuecomment-339238723,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKUN53t5dD3QcG2YtDN6MY2_3-kxzk-Hks5svuG8gaJpZM4QFP8y
.

I'm not sure I really like this approach. How do I assert something was chained in relation to the first job? Doesn't this just test it was chained "somewhere"?

@taylorotwell
Good point, I was thinking to pass something like $parentJob to the callback or to check if  second parameter is a string in assertChained method and compare against that as well.
Queue::assertChained(SecondJob::class, function ($job, $parentJob) {
    // do whatever you want with $parentJob
});
// or
Queue::assertChained(SecondJob::class, FistJob::class);

I decided to skip this because I didn't really think of any good real life examples where this might be applicable. Although I can easily update the PR to match these needs if you want.

I think I'm going to hold off on this for now.


Added PERMAS file format

Please rebase.

There seems to be no further interest in this PR. Closing.


⚠ Processors has degraded performance

In 305b0a6, Processors ($STATUS_URL) experienced degraded performance:

HTTP code: 200
Response time: 80 ms

Resolved: Processors performance has improved in 5ccb623.


The container should support forward resolution

Description
In our codebase, the current dependency container can accept resolution requests for types not yet registered in the container. In our code it returns a "future", but I think to be more correct, we'd return a Promise from the container that would be fulfilled in the future either 1) when the type is registered or 2) on first non-forward resolution (the user should be able to choose between policies)
Alternate solutions
I worked around this by using a separate service within the container to track outstanding resolution requests and resolve them per policy when the types are registered with the container.

Are you thinking something like container.resolve(..., { allowDelayed: true }) which would return a Promise?

That might work - I started looking at this as resolvePromise(...) : Promise, but I got hung up on my idea that we could make it so that when that registration is actually resolvable, the promise would get resolved(i.e., if you try to resolvePromise<Foo>() but Foo's dependencies are not yet registered, nothing happens until eventually they are registered, then the promise completes). I'm not sure that is actually possible. What we have today in my codebase is resolveFuture which returns a Promise that gets resolved when someone calls resolve on the type, allowing the user to take an action before the resolved type is returned to the caller of resolve.
So, I was starting to think more broadly about adding an interception API to TSyringe that would allow for this, though I could simply write it as it works today in resolveFuture. My fear about that is that I'm unsure if it's a generally useful thing, or if it's application specific.


How do I create faux-bold , when bold font is not available as in browser?

I have few fonts like gruppo which does not have bold or italics defined in TTF.How do I create bold or italics (faux one) like how a browser does?

@shyamramesh, I've tried looking at this myself, and it seems there's a small bit missing from the transform given previously to handle text positioned at a non-zero height.
The following works to keep the result from sliding left based on the height of the text for the first line -- the resulting text has a top-left corner in the exact same spot whether the transform is applied or not, but as shown in the screenshot any further lines or text after line wrapping will still be off.

var doc = new PDFDocument();
var stream = doc.pipe(blobStream());

var angle = -20
var skewFactor = Math.tan(angle * Math.PI / 180);
var height = 60;
var size = 72;

doc.fontSize(size)
.fillAndStroke("red")
.text('Here1', 100, height)
.text('and here')
.fillAndStroke("black")
.save()
.text('', 100, height, {continuing: true})
.transform(1, 0, skewFactor, 1, -doc.y*skewFactor, 0)
.text('Here2')
.text('and here')
.restore()

// end and display the document in the iframe to the right
doc.end();
stream.on('finish', function() {
  iframe.src = stream.toBlobURL('application/pdf');
});

Also note that since the top-left corner is pinned, skewing this direction actually pulls the bottom-left corner to the left.  If you want the lower-left corner to start in the same location, then you need to alter the transform matrix further, by an amount that depends on the text/line height.  By trial and error, I've managed to come up with something that seems to work, though I haven't tested it with additional fonts.  It has the same problem with line wraps as the above.

.transform(1, 0, skewFactor, 1, (doc.currentLineHeight(true)-doc.y)*skewFactor-(2*doc.currentLineHeight(false)-doc.currentLineHeight(true))*skewFactor, 0)

@shyamramesh Could you please tell me how did you solve the bold text, I don't know how to apply stroke option to a text?

Calling doc.lineWidth(doc._fontSize * 0.03) before adding the text gives a bold effect.
Dont forget to set the color if necessary: doc.strokeColor(doc._fillColor[0], doc._fillColor[1]);

Guys,
Could you please tell me how do you manage to get the bold using stroke function. I have tried but it does not change. Thanks.

Here is an example
// create a document and pipe to a blob
var doc = new PDFDocument();
var stream = doc.pipe(blobStream());

// draw some text
doc.fontSize(25)
doc.lineWidth(doc._fontSize * 0.03)
doc.strokeColor('black')
doc.fillColor('black')
doc.text('Some ', 100, 80, {continued: true, stroke: false, fill: true})
doc.text('bold', 100, 80, {continued: true, stroke: true, fill: true});
doc.text(' text', 100, 80, {continued: true, stroke: false, fill: true});

// end and display the document in the iframe to the right
doc.end();
stream.on('finish', function() {
  iframe.src = stream.toBlobURL('application/pdf');
});

Thanks alafr. It is really what I need. you saved my day


Add python-scrapinghub package

Review, please.

shouldn't we bump hubstorage version to 0.23.6 here as well?


Remove beta versions of Thunderbird and Firefox

          @stephengillie Please don't approve @vedantmgoyal2009's PRs for `Mozilla.Thunderbird.Beta` anymore, as these cause problems with the current version of winget.

Originally posted by @treysis in https://github.com/microsoft/winget-pkgs/issues/125399#issuecomment-1804295063

These cause problems for customers during the upgrade flow.
It's like we will not be able to safely bring these packages back into the community repository until either the installers have a way for WinGet to determine what versions are stable vs. beta or WinGet has support for channels:

https://github.com/microsoft/winget-cli/issues/147

Mentioning @Trenly as well based on comments in the PR.

The issue can be closed, once the following PRs have been merged.

https://github.com/microsoft/winget-pkgs/pull/126771
https://github.com/microsoft/winget-pkgs/pull/126772
https://github.com/microsoft/winget-pkgs/pull/126773
https://github.com/microsoft/winget-pkgs/pull/126774
https://github.com/microsoft/winget-pkgs/pull/126775

Close with reason: Beta versions of firefox and thunderbird have been removed;


Prediction failed for an unknown reason

I have train the yolov model in cog but it shows below error.. @zeke @mattt
Freezing layer 'model.22.dfl.conv.weight'
AMP: running Automatic Mixed Precision (AMP) checks with YOLOv8n...
{"logger": "cog.server.runner", "timestamp": "2024-06-06T07:43:10.072634Z", "exception": "Traceback (most recent call last):\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/runner.py\", line 141,
in handle_error\n    raise error\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/multiprocessing/pool.py\", line 125, in worker\n    result = (True, func(*args, **kwds))\n  File \"/root/.pyenv/versions/3.10.14/lib/python3
.10/site-packages/cog/server/runner.py\", line 371, in predict\n    return _predict(\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/runner.py\", line 413, in _predict\n    for event in worker.pre
dict(input_dict, poll=0.1):\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/worker.py\", line 139, in _wait\n    raise FatalWorkerException(\ncog.server.exceptions.FatalWorkerException: Prediction
 failed for an unknown reason. It might have run out of memory? (exitcode -11)", "severity": "ERROR", "message": "caught exception while running prediction"}
{"prediction_id": null, "logger": "uvicorn.error", "timestamp": "2024-06-06T07:43:10.075073Z", "exception": "Traceback (most recent call last):\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/uvicorn/protoco
ls/http/httptools_impl.py\", line 399, in run_asgi\n    result = await app(  # type: ignore[func-returns-value]\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py\", line 70,
 in __call__\n    return await self.app(scope, receive, send)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/fastapi/applications.py\", line 284, in __call__\n    await super().__call__(scope, receive, send
)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/starlette/applications.py\", line 122, in __call__\n    await self.middleware_stack(scope, receive, send)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3
.10/site-packages/starlette/middleware/errors.py\", line 184, in __call__\n    raise exc\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/starlette/middleware/errors.py\", line 162, in __call__\n    await sel
f.app(scope, receive, _send)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/starlette/middleware/exceptions.py\", line 79, in __call__\n    raise exc\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/s
ite-packages/starlette/middleware/exceptions.py\", line 68, in __call__\n    await self.app(scope, receive, sender)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py\", lin
e 20, in __call__\n    raise e\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py\", line 17, in __call__\n    await self.app(scope, receive, send)\n  File \"/root/.pyenv/ve
rsions/3.10.14/lib/python3.10/site-packages/starlette/routing.py\", line 718, in __call__\n    await route.handle(scope, receive, send)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/starlette/routing.py\",
 line 276, in handle\n    await self.app(scope, receive, send)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/starlette/routing.py\", line 66, in app\n    response = await func(request)\n  File \"/root/.pye
nv/versions/3.10.14/lib/python3.10/site-packages/fastapi/routing.py\", line 241, in app\n    raw_response = await run_endpoint_function(\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/fastapi/routing.py\",
line 167, in run_endpoint_function\n    return await dependant.call(**values)\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/http.py\", line 292, in predict\n    return _predict(\n  File \"/root/
.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/http.py\", line 370, in _predict\n    response = PredictionResponse(**async_result.get().dict())\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/multiprocess
ing/pool.py\", line 774, in get\n    raise self._value\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/runner.py\", line 141, in handle_error\n    raise error\n  File \"/root/.pyenv/versions/3.10.
14/lib/python3.10/multiprocessing/pool.py\", line 125, in worker\n    result = (True, func(*args, **kwds))\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/runner.py\", line 371, in predict\n    re
turn _predict(\n  File \"/root/.pyenv/versions/3.10.14/lib/python3.10/site-packages/cog/server/runner.py\", line 413, in _predict\n    for event in worker.predict(input_dict, poll=0.1):\n  File \"/root/.pyenv/versions/3.10.14/li
b/python3.10/site-packages/cog/server/worker.py\", line 139, in _wait\n    raise FatalWorkerException(\ncog.server.exceptions.FatalWorkerException: Prediction failed for an unknown reason. It might have run out of memory? (exitc
ode -11)", "severity": "ERROR", "message": "Exception in ASGI application\n"}

my cuda deatils below..
Ultralytics YOLOv8.2.28 🚀 Python-3.10.14 torch-2.3.0+cu118 CUDA:0 (NVIDIA GeForce RTX 4090, 24564MiB)

cog.yaml file
build:
  gpu: True
  cuda: "11.8"
  python_version: "3.10"
  system_packages:
    - "cmake"
    - "ffmpeg"
    - "libsm6"
    - "libxext6"
    - "libgl1-mesa-glx"
    - "libglib2.0-0"
  python_packages:
    - "ultralytics==8.2.28"
    - "torch==2.3.0"
    - "requests==2.32.3"
    - "tqdm==4.66.4"
predict: "predict.py:Predictor"

Hi @tzktok. From those error logs:

Prediction failed for an unknown reason. It might have run out of memory?

It sounds like your app crashed due to an OOM.


Update estimator.py, cpu integration

Couldn't run the code using cpu because device in estimator.py was hardcoded in cuda initially. changed to a dynamic version.

Thank you @ilbeygulmez23 !


Issue with Journals Plugin: TypeError: this.item.key.intersect is not a function

Issue with Journals Plugin: TypeError: this.item.key.intersect is not a function
Description
I am experiencing a critical issue with the Journals plugin in Obsidian. The plugin is throwing a TypeError: this.item.key.intersect is not a function in the developer console, which prevents the creation of new journal entries via the calendar. This issue occurs even after disabling almost all other plugins except for DataView, Journals, and Templater. Below is a detailed breakdown of the setup and error logs.
Setup Details

Obsidian Version: 1.6.5
Active Plugins:

DataView
Journals
Templater

Folder Structure

Root Level Folder: 80-Journal
Subfolders:

01-Daily
02-Weekly
03-Monthly
04-Quarterly
05-Yearly

Template Folder: 90-Vault-Setup\Sys-Data-Templates\80-Journal

Steps to Reproduce

Open Obsidian and enable the DataView, Journals, and Templater plugins.
Ensure that the journal folder structure is set as described above.
Attempt to create a new journal entry via the calendar.

Expected Behavior
A new journal entry should be created successfully without any errors.
Actual Behavior
The plugin fails to create a new journal entry, and the following error is displayed in the developer console:
Uncaught (in promise) TypeError: this.item.key.intersect is not a function
    at Node.intersect (plugin:journals:2174:30)
    at IntervalTree.tree_search_interval (plugin:journals:2653:22)
    at IntervalTree.search (plugin:journals:2341:14)
    at IntervalTree.find (plugin:journals:2344:14)
    at CalendarViewMonth.updateNotesIndex (plugin:journals:6282:41)
    at CalendarViewMonth.eval (plugin:journals:6140:18)
    at Generator.next (<anonymous>)
    at eval (plugin:journals:28:71)
    at Promise (<anonymous>)
    at asyncGeneratorStep (plugin:journals:24:12)
    at _await (plugin:journals:24:12)

Request for Assistance
Please investigate this issue and provide a fix or workaround. The current plugin behavior severely impacts my ability to use the Journals plugin effectively.
Thank you for your assistance.

Attachments:

[2024-06-30 07_24_29-New tab - AIFusionVision - Obsidian v1.6.5.png](attachment_path/2024-06-30 07_24_29-New tab - AIFusionVision - Obsidian v1.6.5.png)

I found the problem - your screenshots helped a lot.
This happens because in your template you have journal related properties in frontmatter with variable instead of date
like
---
journal: Journal
journal-start-date: {{date}}
journal-end-date: {{date}}
journal-section: day
---

Just remove them in template - plugin will insert them when new note created.
If it does not help right away - close and open Obsidian (to reset plugin state).
I'll add more check to prevent such failures in next release

Thank you. The useful tips provided resolved this issue. Your assistance has been invaluable. It's working great now.


CORE-5907: Refactor endpoints to use single sender service Pt. 1

First part of two part PR. Initial PR should be a statement of intent around how the virtual node endpoints will work.
In the current PR, the RPC Sender is abstracted into it's own service. This service is then used by the two parent services. In the second pr, this will be replaced by a factory OSGI service without any lifecycle, and the lifecycle management code will be moved back into the parents to prevent a situation where one parent service will stop the sender service thereby breaking the other service.
In this case, this is probably okay as the http endpoints both operate on the same object, and have the same requirements, but it's been deemed better not to establish this anti-pattern in code over the longer term

@kyriathar Decided to leave those suggestions for part two if that's alright with you!


[fix][broker] Create new ledger after the current ledger is closed

Motivation
Background

Managerledger has a configuration maximumRolloverTimeMs to configure the max time of closing a ledger.
The current ledger can be rollover when it reaches maximumRolloverTimeMs.
The current ledger will not change after it is rollovered. The current ledger only be changed when creating a new ledger.
The current ledger can not be deleted, because the current ledger may be written to.
The ClosedLedger state of the manager indicates that the current ledger is rollovered and can not be written to.

In the current logic, if it is found that the current ledger should be closed during the process of writing an entry, then the current ledger will be closed after this entry is written. However, if there is no pending add entry operation, a new ledger will not be created. This will cause the current ledger to be unable to be deleted, and the retention policy cannot be executed as expected by the user. This is the problem we need to solve, and I will further elaborate on this issue below:
We need to open a new ledger after the current ledger when it is rolled over. In fact, Pulsar has a task that periodically checks if the ledger is full, and it will create a new ledger immediately after closing the ledger. Subsequently, the new ledger becomes the current ledger and the previous current ledger can be deleted.
https://github.com/apache/pulsar/blob/fc2e314c3560eb5011ab3e5e3ebf66fa0b9e6d4e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L4445-L4447
https://github.com/apache/pulsar/blob/fc2e314c3560eb5011ab3e5e3ebf66fa0b9e6d4e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1794-L1816
However, the ledger may also be closed after adding an entry, and at this time, if there is no pending add entry operation, a new ledger will not be created. Therefore, the already closed current ledger cannot be deleted because there is no new ledger to become the current ledger.
https://github.com/apache/pulsar/blob/fc2e314c3560eb5011ab3e5e3ebf66fa0b9e6d4e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java#L276
https://github.com/apache/pulsar/blob/fc2e314c3560eb5011ab3e5e3ebf66fa0b9e6d4e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1757-L1760
Modifications
We have two ways to solve this problem:

Do not skip the current ledger when trimming ledgers.
Create a new ledger after the current ledger is closed and there are no pending add entry operations.

In most cases, users take more care about the write-read latency instead of the cost of storage brought by creating an empty ledger. So I suggest we delete the limit of !pendingAddEntries.isEmpty(), creating a new ledger after closing the current ledger.
Verifying this change

[ ] Make sure that the change passes the CI checks.

(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

[ ] Dependencies (add or upgrade a dependency)
[ ] The public API
[ ] The schema
[ ] The default values of configurations
[ ] The threading model
[ ] The binary protocol
[ ] The REST endpoints
[ ] The admin CLI options
[ ] The metrics
[ ] Anything that affects deployment

Documentation

[ ] doc
[ ] doc-required
[x] doc-not-needed
[ ] doc-complete

Matching PR in forked repository
PR in forked repository:

This sounds like a big behaviour change. What about discussing on the dev@ mailing list ?

ClosedLedger means Current ledger has been closed , so we can delete it. But I want to enable the test and check if it works

The current ledger will not change after it is rollovered. The current ledger only be changed when creating a new ledger.

Hi, @liangyepianzhou  could you please point out which code? I saw we will create a new ledger here. https://github.com/apache/pulsar/blob/0dabc9754aa371b241fc3daec4df21bee3ff8ce2/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1815

Oh, I saw one place will cause this behaviour https://github.com/apache/pulsar/blob/0dabc9754aa371b241fc3daec4df21bee3ff8ce2/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L859. But it should be okay, the rollover will create a new ledger finally.

Oh, I saw one place will cause this behaviour
https://github.com/apache/pulsar/blob/0dabc9754aa371b241fc3daec4df21bee3ff8ce2/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L859
. But it should be okay, the rollover will create a new ledger finally.

Oh, I see. This is a bug, not a behavior change. The rollover in the OpAddEntry does not create a new ledger.
https://github.com/apache/pulsar/blob/0dabc9754aa371b241fc3daec4df21bee3ff8ce2/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java#L244-L249

Is this related to #21893?

@liangyepianzhou It would be good to improve the description of the PR.

We need to delete the current ledger when it is rollovered.

This doesn't make sense.

Do not skip the current ledger when trimming ledgers if the state of the managerLedger is ClosedLedger.

This makes more sense. Is this what is the actual goal? Obviously the trimming could result in deleting the ledger.

Would it make sense to rename "Delete current ledger when it is closed" to "Trim current ledger when it is closed" if that's what is the expectation?

It seems that the problem might be very different. It would be necessary to explain the use case.
The description doesn't explain the use case. If I have understood correctly, the use case is about
configuring retention by time. When retention is configured, it is expected that expired messages get deleted.
That isn't happening in some cases currently and that's the issue that is being addressed.
One rare case where Pulsar currently seems to ignore retention policies is the case when topics aren't actively loaded on any broker in the cluster. A topic gets loaded when a consumer or producer connects to the topic.

There's a scheduled job https://github.com/apache/pulsar/blob/5df97b41a431a714a448c198c312e98aa4a084e8/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L653-L659 that runs by default every 120 seconds (configured with retentionCheckIntervalInSeconds).
That iterates all active topics and their managed ledgers:
https://github.com/apache/pulsar/blob/5df97b41a431a714a448c198c312e98aa4a084e8/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L2041-L2051
This works as long as the topic is active (loaded) in a broker in the cluster.

@lhotari Thanks for your review. It helps a lot. I have updated the description, which has greatly changed from my original idea. In the initial plan, I did not consider the following two points:

Creating a new ledger might be more acceptable to users.
The existing timed task also directly creates a new ledger, so we should maintain consistency in behavior.

I hope the original description did not cause you too much trouble, and I look forward to your reply.

I wonder if we should close the old one and create a new one if the last ledger is empty.

This case should not happen.
For timed tasks, call rollCurrentLedgerIfFull. It will check the ledger is not empty.
        if (currentLedgerEntries > 0 && currentLedgerIsFull()
                && STATE_UPDATER.compareAndSet(this, State.LedgerOpened, State.ClosingLedger)) {
                ...
        }

For the add-entry process, the ledger will be closed after the entry is added completely.

Hi, guys.
I can't say the new method has problems so far. I just worry that we have broken the lazy ledger creation logic.
How about changing the rollCurrentLedgerIfFull method to be as follows? The schedule rollover checker will roll the current closed ledger after maximumRolloverTimeMs
   public void rollCurrentLedgerIfFull() {
        log.info("[{}] Start checking if current ledger is full", name);
        if (currentLedgerEntries <= 0 || !currentLedgerIsFull()) {
            return;
        }
        if(STATE_UPDATER.compareAndSet(this, State.LedgerOpened, State.ClosingLedger)) {
            currentLedger.asyncClose(new AsyncCallback.CloseCallback() {
                @Override
                public void closeComplete(int rc, LedgerHandle lh, Object o) {
                    checkArgument(currentLedger.getId() == lh.getId(), "ledgerId %s doesn't match with "
                                                                       + "acked ledgerId %s", currentLedger.getId(), lh.getId());

                    if (rc == BKException.Code.OK) {
                        if (log.isDebugEnabled()) {
                            log.debug("[{}] Successfully closed ledger {}, trigger by rollover full ledger",
                                    name, lh.getId());
                        }
                    } else {
                        log.warn("[{}] Error when closing ledger {}, trigger by rollover full ledger, Status={}",
                                name, lh.getId(), BKException.getMessage(rc));
                    }

                    ledgerClosed(lh);
                    createLedgerAfterClosed();
                }
            }, null);
        } else if (STATE_UPDATER.get(this) == State.ClosedLedger) {
            createLedgerAfterClosed();
        }
    }

@liangyepianzhou this change added a new flaky test, #22347 . do you have a change to fix it?

@liangyepianzhou never mind, flaky test issue is already fixed by #22344

@liangyepianzhou this added a flaky test #22351, do you have a chance to fix it? thanks

When cherry-picking, it's important to also pick #22552

This PR introduced a flaky test #23164, @liangyepianzhou do you have a chance to fix it? thanks


GoodMarket: The G$ symbol overlaps the item's price on the "Add new item's price" screen on GoodMarket

User is logged in Goodmarket. User is adding new item
Steps to reproduce:

Upload the images and tap the "Next" button
Add the item' description and tap the "Next" button
Pay attention how G$ symbol and placeholder are displayed
Add the price in the price field
Observe the result

Actual result: The G$ symbol overlaps the item's price on the "Add new item's price" screen
Expected result: The G$ symbol and item's price are displayed correctly on the "Add new item's price" screen
Note: buttons aren't work both for iOS and Android devices
Xiaomi Redmi 7 // Android 9.0
iPhone XS // iOS 13.1

should be fixed by paperclip

Not reproduced. The G$ symbol and price are displayed correctly now


Add method stringifyVariable to use method depending on sf version

In sf >= 4, the DataCollector::varToString() doesn't exists anymore.
Add a proxy method to check the availability of the method

thanks @mojoLyon 👍


This merges in 4 of the 5 open PRs

The PR from UnanimousCoward was too old to merge, it had conflicts with the others

Incorporated with IndicatorLights Community Extensions 1.6.1.


📖 DOC: Write README

This should become easier after #9
Write up docs on how to install and use. Since we're not trying for official status, try and keep the README as the only docs. This may eventually become impossible, but we'll see over time.

Done in #29