structural recursion fails in the equation compiler

Prerequisites
Please put an X between the brackets as you perform the following steps:

[X] Check that your issue is not already filed:
https://github.com/leanprover/lean4/issues
[X] Reduce the issue to a minimal, self-contained, reproducible test case.
Avoid dependencies to Mathlib or Batteries.
[X] Test your test case against the latest nightly release, for example on
https://live.lean-lang.org/#project=lean-nightly
(You can also use the settings there to switch to “Lean nightly”)

Description
variable {α : Type}
variable (r : α → α → Prop)

/-- `SymmGen r` is the symmetric relation generated by `r`. -/
inductive SymmGen : α → α → Prop
  | rel : ∀ x y, r x y → SymmGen x y
  | symm : ∀ x y, SymmGen x y → SymmGen y x

def MyRel : Nat → Nat → Prop := SymmGen fun x y => y = x + 2

theorem preserve_add' {a : Nat} : ∀ {x y : Nat}, MyRel x y → MyRel (x + a) (y + a)
| _, _, SymmGen.rel _ _ h => SymmGen.rel _ _ (by rw [h, Nat.add_right_comm])
| _, _, SymmGen.symm _ _ h => SymmGen.symm _ _ (preserve_add' h)

The second case errors, despite being applied to a structurally smaller term.
Context
Discussion thread
Steps to Reproduce

Code as above.

Expected behavior:
Code compiles with no error.
Actual behavior:
Code errors, with error message
fail to show termination for
  preserve_add'
with errors
argument #2 was not used for structural recursion
  failed to eliminate recursive application
    preserve_add' h

argument #3 was not used for structural recursion
  failed to eliminate recursive application
    preserve_add' h

argument #4 was not used for structural recursion
  could not add below discriminant

structural recursion cannot be used

Could not find a decreasing measure.
The arguments relate at each recursive call as follows:
(<, ≤, =: relation proved, ? all proofs failed, _: no proof attempted)
            x y
1) 13:48-63 ? ?
Please use `termination_by` to specify a decreasing measure.

Versions
Same behaviour on:
live.lean-lang.org current (4.9.0-rc2)
live.lean-lang.org nightly (4.10.0-nightly-2024-06-23)
MacOS Monterey 4.9.0-rc1
Additional Information
Another example:
variable {Tm : Type}

inductive Steps (Step : Tm → Tm → Prop): Tm → Tm → Prop :=
  | trans {a b c} : Step a b → Steps Step b c → Steps Step a c

theorem stepsTrans {Step} {a b c : Tm} (r₁ : Steps Step a b) (r₂ : Steps Step b c) : Steps Step a c :=
  match r₁ with
  | Steps.trans r r₁ => Steps.trans r (stepsTrans r₁ r₂)

Impact
Add :+1: to issues you consider important. If others are impacted by this issue, please ask them to add :+1: to it.

Thanks! Does the former example depend on the def MyRel definition to trigger the issue?

I think so, in the sense that I don't know how to modify the example to avoid that definition.

I'm on the phone so I can't try, but can you not unfold it in the type of preserve_add'?

Ah, I must have looked at it before in April and found:
Based on the available trace options, the issue is somewhere in
def findBelowIdx (xs : Array Expr) (motive : Expr) : MetaM $ Option (Expr × Nat) := do
  xs.findSomeM? fun x => do
  let xTy ← inferType x
  xTy.withApp fun f _ =>
  match f.constName?, xs.indexOf? x with
  | some name, some idx => do                                                                       if (← isInductivePredicate name) then
      let (_, belowTy) ← belowType motive xs idx                                                      let below ← mkFreshExprSyntheticOpaqueMVar belowTy
      try
        trace[Meta.IndPredBelow.match] "{←Meta.ppGoal below.mvarId!}"
        if (← backwardsChaining below.mvarId! 10) then
          trace[Meta.IndPredBelow.match] "Found below term in the local context: {below}"
          if (← xs.anyM (isDefEq below)) then pure none else pure (below, idx.val)                      else
          trace[Meta.IndPredBelow.match] "could not find below term in the local context"
          pure none
      catch _ => pure none
    else pure none
  | _, _ => pure none

(Adding this here just for future reference)

Woah, I see! The former example does depend on the definition to trigger the issue: unfolding it manually as you say makes the error disappear.
Marking the def @[inline, reducible] does not make the error disappear.

Thanks for checking, this will certainly help whenever someone gets around debugging this :-)

It also makes me wonder if the second example has the same cause, as no def is involved there.

@nomeata Yes, both examples are hitting bugs in the structural predicate recursion module: IndPred.lean.
@DanielFabian Any ideas on what might be the problem?
@b-mehta Possible workaround: define SymmGen and Steps in Type. It is not great, but it will work around the problem.
variable {α : Type}
variable (r : α → α → Prop)

/-- `SymmGen r` is the symmetric relation generated by `r`. -/
inductive SymmGen : α → α → Type where
  | rel : ∀ x y, r x y → SymmGen x y
  | symm : ∀ x y, SymmGen x y → SymmGen y x

def MyRel : Nat → Nat → Type := SymmGen fun x y => y = x + 2

def preserve_add' {a : Nat} : ∀ {x y : Nat}, MyRel x y → MyRel (x + a) (y + a)
| _, _, SymmGen.rel _ _ h => SymmGen.rel _ _ (by rw [h, Nat.add_right_comm])
| _, _, SymmGen.symm _ _ h => SymmGen.symm _ _ (preserve_add' h)

variable {Tm : Type}

inductive Steps (Step : Tm → Tm → Prop): Tm → Tm → Type :=
  | trans {a b c} : Step a b → Steps Step b c → Steps Step a c

def stepsTrans {Step} {a b c : Tm} (r₁ : Steps Step a b) (r₂ : Steps Step b c) : Steps Step a c :=
  match r₁ with
  | Steps.trans r r₁ => Steps.trans r (stepsTrans r₁ r₂)

Thanks for the workaround! The one I went with was to use the induction tactic, which worked for me instead. But I still find the equation compiler more aesthetically pleasing, hence this report.

Without debugging my first guess would be that it fails in backwards chaining. That's also the root cause of the other bug ypu tagged me on. Basically, backwards chaining is greedy and not exhaustive iirc. We wanted to use a more powerful tactic here, i believe. Would it be possible to the proof state as its trying to do the backward chaining?

adding #1672 as a reference. We may well have the same problem; I need to figure out how I got the proof state in the other case. But if my guess is right, we are again in a situation where the compiler picks the wrong one of a few possible objects and as that adjusts the goals they become unprovable.
The solution would be to try all applicable objects of the correct type and backtrack.

Ok, I did find at least, that it's probably not a problem with backwards chaining, as the code doesn't even seem to get to the first trace line in the try block.

@leodemoura I'd need a bit of a crash course in how to debug lean again if I'm to try and find what's going on. It doesn't seem to even get to the tricky bit of synthesizing a proof.

So this particular issue, the one that goes away when unfolding MyRel, clearly points to missing calls to whnf in the code. And indeed with this the example works:
diff --git a/src/Lean/Meta/IndPredBelow.lean b/src/Lean/Meta/IndPredBelow.lean
index b9fc37fc53..641302b08d 100644
--- a/src/Lean/Meta/IndPredBelow.lean
+++ b/src/Lean/Meta/IndPredBelow.lean
@@ -377,7 +377,7 @@ where
       loop xs rest belowIndices xIdx (yIdx + 1)

 private def belowType (motive : Expr) (xs : Array Expr) (idx : Nat) : MetaM $ Name × Expr := do
-  (← inferType xs[idx]!).withApp fun type args => do
+  (← whnf (← inferType xs[idx]!)).withApp fun type args => do
     let indName := type.constName!
     let indInfo ← getConstInfoInduct indName
     let belowArgs := args[:indInfo.numParams] ++ #[motive] ++ args[indInfo.numParams:] ++ #[xs[idx]!]
@@ -554,8 +554,7 @@ where

 def findBelowIdx (xs : Array Expr) (motive : Expr) : MetaM $ Option (Expr × Nat) := do
   xs.findSomeM? fun x => do
-  let xTy ← inferType x
-  xTy.withApp fun f _ =>
+  (← whnf (← inferType x)).withApp fun f _ =>
   match f.constName?, xs.indexOf? x with
   | some name, some idx => do
     if (← isInductivePredicate name) then

@DanielFabian, does that look reasonably? Are you interested in PR’ing that one (presumably after going through the code and checking if there are other places where you infer a type and should whnf before looking for an inductive predicate?
While we are at it: I am wondering if the IndBelow code could be unified with the BRecOn code some more. For example it would greatly simplify things if instead of constructing a fresh matcher, we could use Lean.Meta.MatcherApp.addArg like we do in the BRecOn construction. Presumably this would refine the type of the below value just like there … maybe the only difference is that instead of using basic projection (and reduction) to get the value out of the below, we’d need to generate appropriately typed projections for each field in each constructor of the below inductive and use these.
Maybe this would even get rid some of the heuristics and tactics? (The BRecOn code doesn’t need those, after all.)

Do you understand how the construction works? It weaves these smaller witnesses into each of the arguments of the constructors of the predicate. And we prove brecOn for that other predicate. It works really quite differently from the below construction for type valued objects. The idea is that we create a stronger match clause and therefore can read off a below witness.
When I was doing it a few years ago, there didn't seem to be a good way to treat prop and type similarly.
Ultimately it boils down to proof irrelevance that does not allow us to extract any values out of a prop. So instead I need to define a whole parallel structure that's richer than the original prop and carries witnesses in the construction.
Happy to hop on a call and chat about it, if you like.

I have a cold right now, so not too keen on videocalls. I’ll also familiarize myself with the code, and try any possible construction by hand first, more before making more such proposals :-)

honestly no problem, it's a bit complicated and I don't currently remember the whole detail. But essentially it goes more or less like this:
inductive MyProp : Prop
| Nil : MyProp
| Cons : (a: MyProp) → MyProp

for this we'd define an auxiliary prop more or less like this:
inductive MyProp.below : Prop
| Nil : MyProp.below
| Cons : (a: MyProp) → (a_below: below a) → MyProp.below

the a_below is a witness that a is smaller than the current one (sorry, don't remember the correct type at the moment). But crucially we rewrite your original pattern match into a pattern match on top of the MyProp.below type. That works, because it carries all the same information + additionally the below witnesses.
So when you are trying to eliminate the structural recursion and prove the below a property, it's just straight away available in the local ctx. The price we pay, is that we now need to prove the below a property, iirc. And for that we use the backwards chaining.
Again, sorry, if my memory is not 100% accurate, it's been a few years and it was a bit technical.

Right, so far I could follow. And the MyProp.below has the same purpose as the MyType.below, they have very similar types and semantic content.
So my thought was:  Can we leave the .brecOn and .below construction for MyProp as it is, but change the transformation from PreDefinition to a call to .brecOn to be as similar as the one for non-prop recursion.
So in Type we have
inductive N' : Nat → Type where
  | zero : N' 0
  | succ : N' n -> N' (n + 1)

def preserve_succ' : (n : Nat) → N' n → N' (n + 1)
  | .zero, .zero => .succ .zero
  | .succ n, .succ hn => .succ (preserve_pred' n hn)

and get this implementation under the hood
def preserve_succ'_impl : {n : Nat} → N' n → N' (n + 1) := fun {n} x =>
  N'.brecOn x fun {n} x f =>
    (match (motive := (n : Nat) → (x : N' n) → N'.below x → N' (n + 1)) n, x with
      | _, N'.zero => fun x => N'.zero.succ
      | _, N'.succ hn => fun x => N'.succ x.fst.fst)
      f

Note that (curcially!) this match is the original match from preserve_succ, there is no need to create a new match definition. Instead, the N'.below value is added as an “extra argument” and mentioned in the motive, so that it's type in the arms of the match is refined, and x.fst.fst picks out the motive stored in the below.
I have hopes that the same construction should work in the Prop case as well. Refining the index of the .below as we traverse into matches should work just the same. We only™ need to change x.fst.fst and replace these PProd projections with functions that project the values out of a N.below (with known index):
inductive N : Nat → Prop where
  | zero : N 0
  | succ : N n -> N (n + 1)

theorem N.below.succ_1 {n : Nat} {motive : (a : Nat) → N a → Prop} (hn1 : N (n + 1)) :
  @N.below motive (n + 1) hn1 → N n
  | @N.below.succ _ _ hn _ _ => hn

theorem N.below.succ_2 {n : Nat} {motive : (a : Nat) → N a → Prop} (hn1 : N (n + 1)) :
  (nb : @N.below motive (n + 1) hn1) → @N.below motive n nb.succ_1
  | @N.below.succ _ _ _ hb _ => hb

theorem N.below.succ_3 {n : Nat} {motive : (a : Nat) → N a → Prop} (hn1 : N (n + 1)) :
  (nb : @N.below motive (n + 1) hn1) → motive n nb.succ_1
  | @N.below.succ _ _ _ _ x => x

theorem preserve_succ_impl : (n : Nat) → N n → N (n + 1) := fun x x_1 =>
  N.brecOn x_1 fun x x_2 f =>
    (match (motive := (x : Nat) → (x_3 : N x) → N.below x_3 → N (x + 1)) x, x_2 with
      | Nat.zero, N.zero => fun x => N.zero.succ
      | Nat.succ n, N.succ hn => fun x => N.succ x.succ_3)
      f

Does this look reasonable?

correct me if I'm wrong, but you can't project anything out of a prop due to proof irrelevance?

You can as long as you project into Prop! But the motive and the N.below is in Prop, so that should be fine.

i wonder, then, why it didn't work when we were trying to do that a few years ago. Please do try your construction also on this one:
https://github.com/leanprover/lean4/blob/18c97926a13bfc82c9266bf00483ab13c6acb64f/tests/lean/run/inductive_pred.lean#L38
It was one of the motivating examples, I think.

(obviously if we find that your much simpler construction works, then let's ditch mine as it would be needlessly complicated - but let's please not regress what we can prove)

Ah, thanks, that’s very helpful, and indeed I can’t make it work.
It seems it boils down to the fact that that Lean cannot tell that a value x : Power2.below (Power2.ind h) must have been constructed with the Power2.below.ind constructor, because of proof irrelevance of Power2.

 My attempt
-- Status Quo

inductive Power2 : Nat → Prop
  | base : Power2 1
  | ind  : Power2 n → Power2 (2*n) -- Note that index here is not a constructor

theorem Power2.mul : Power2 n → Power2 m → Power2 (n*m)
  | h1, base   => (Nat.mul_one n).symm ▸ h1
  | h1, ind h2 => (Nat.mul_left_comm ..) ▸ .ind (mul h1 h2)

-- Construction in Type

inductive Power2' : Nat → Type
  | base : Power2' 1
  | ind  : Power2' n → Power2' (2*n) -- Note that index here is not a constructor

def Power2'.mul : Power2' n → Power2' m → Power2' (n*m)
  | h1, base   => (Nat.mul_one n).symm ▸ h1
  | h1, ind h2 => (Nat.mul_left_comm ..) ▸ .ind (mul h1 h2)

set_option pp.match false
set_option pp.proofs true

#print Power2'.mul

-- This is the construciton used in Type

noncomputable
def Power2'.mul_impl : {n m : Nat} → Power2' n → Power2' m → Power2' (n * m) :=
fun {n m} x x_1 =>
  Power2'.brecOn (motive := fun {m} x => Power2' n → Power2' (n * m)) x_1
    (fun {m} x f x_2 =>
      Power2'.mul.match_1
        (fun m x x => Power2'.below (motive := fun {m} x => Power2' n → Power2' (n * m)) x → Power2' (n * m)) m x_2 x
        (fun h1 x => (Nat.mul_one n).symm ▸ h1)
        (fun h1 n_1 h2 x => (Nat.mul_left_comm ..) ▸ (x.fst.fst h1).ind) f)
    x

-- To replicate it in Prop, we need a replacement for x.fst.fst
-- If we had a function that can digest a value of type `Power2.below (Power2.ind h)` we could do it:

theorem Power2.below.ind_2_wish {n : Nat} {motive : (a : Nat) → Power2 a → Prop} {h : Power2 n} :
  (nb : @Power2.below motive (2 * n) (.ind h)) → motive n h := sorry

theorem Power2.mul_impl : {n m : Nat} → Power2 n → Power2 m → Power2 (n * m) :=
fun {n m} x x_1 =>
  @Power2.brecOn (fun {m} x => Power2 n → Power2 (n * m)) _ x_1
    (fun {m} x f x_2 =>
      Power2.mul.match_1
        (fun m x x => @Power2.below (fun {m} x => Power2 n → Power2 (n * m)) _ x → Power2 (n * m)) m x_2 x
        (fun h1 x => (Nat.mul_one n).symm ▸ h1)
        (fun h1 n_1 h2 x => (Nat.mul_left_comm ..) ▸ (.ind (@below.ind_2_wish _ _ h2 x h1))) f)
    x

-- But, as Daniel rightfully expeted, we cannot write that function, due to prop irrelevance

-- tactic 'cases' failed, nested error:
-- dependent elimination failed, failed to solve equation
--   Nat.mul 2 n = 1
theorem Power2.below.ind_2 {n : Nat} {motive : (a : Nat) → Power2 a → Prop} {h : Power2 n} :
  (nb : @Power2.below motive (2 * n) (.ind h)) → motive n h
  | @Power2.below.ind _ _ _ _ x => x

Thanks for bearing with me!

argh, please do
```lean
[lean-code]
 ```

so we all benefit from syntax highlighting ;)

True true, been spoiled by zulip’s default.

It seems it boils down to the fact that that Lean cannot tell that a value x : Power2.below (Power2.ind h) must have been constructed with the Power2.below.ind constructor, because of proof irrelevance of Power2.

I don't think that's even true. Isn't it that due to proof irrelevance, the no-confusion rule is not applicable and you can in fact prove that Power2.below (Power2.ind h) = Power2.below (Power2.base)?

I forgot where exactly we stand with the issue here.
Thanks to termination_by structural we can see that the two issues in the PR description are likely separate: The first gives
failed to infer structural recursion:
Cannot use parameter #4:
  could not add below discriminant

and the second
failed to infer structural recursion:
Cannot use parameter r₁:
  the type Steps Step does not have a `.brecOn` recursor

and with tracing
failed to prove brecOn for Steps.below
    couldn't solve by backwards chaining (Lean.Meta.IndPredBelow.maxBackwardChainingDepth = 10):
α: Type
r: α → α → Prop
Tm✝Tm: Type
Step: Tm → Tm → Prop
motive✝: (a a_1 : Tm) → Steps Step a a_1 → Prop
a✝³a✝²: Tm
x✝: Steps Step a✝³ a✝²
ih✝: ∀ (a a_1 : Tm) (x : Steps Step a a_1), x.below → motive✝ a a_1 x
abc: Tm
a✝¹: Step a b
a✝: Steps Step b c
a_ih✝: a✝.below
⊢ Step a a✝³

In fact, the second example seems to be #1672, which has a possible (but to be refined) fix in #4840, so I’ll just remove that second example from the issue description here.

Oh, and since in the example works if MyRel is inlined, as so
theorem preserve_add' {a : Nat} : ∀ {x y : Nat}, SymmGen (fun x y => y = x + 2) x y → MyRel (x + a) (y + a)
| _, _, SymmGen.rel _ _ h => SymmGen.rel _ _ (by rw [h, Nat.add_right_comm])
| _, _, SymmGen.symm _ _ h => SymmGen.symm _ _ (preserve_add' h)
termination_by structural _ _ x =>

maybe I should downgrade the severity to low.


[CH] fix orc strcut column mismatch after written

Backend
CH (ClickHouse)
Bug description
test(
    "GLUTEN-8021/8022: fix orc read/write mismatch and parquet" +
      "read exception when written complex column contains null") {
    def table(format: String): String = s"t_8021_$format"
    def create(format: String, table_name: Option[String] = None): String =
      s"""CREATE TABLE ${table_name.getOrElse(table(format))}(
         |-- id int,
         |-- x int,
         |-- y int,
         |-- mp map<string, string>,
         |-- arr array<int>,
         |-- tup struct<x:int, y:int>
         |tup struct<x:int>
         |-- arr_mp array<map<string, string>>,
         |-- mp_arr map<string, array<int>>
         |) stored as $format""".stripMargin
    def insert(format: String, table_name: Option[String] = None): String =
      s"""INSERT OVERWRITE TABLE ${table_name.getOrElse(table(format))}
         |with data_source as (
         |select
         |id,
         |if(id % 3 = 1, null, id+1) as x,
         |if(id % 3 = 1, null, id+2) as y
         |from range(100)
         |)
         |select
         |-- id,
         |-- id, x, y,
         |-- str_to_map(concat('x:', x, ',y:', y)) as mp,
         |-- if(id % 4 = 0, null, array(x, y)) as arr,
         |if(id % 4 = 1, null, struct(x)) as tup
         |-- if(id % 4 = 2, null, array(str_to_map(concat('x:', x, ',y:', y)))) as arr_mp,
         |-- if(id % 4 = 3, null, map('x', array(x), 'y', array(y))) as mp_arr
         |from
         |data_source;""".stripMargin

    nativeWrite2(
      format => (table(format), create(format), insert(format)),
      (table_name, format) => {
        val vanilla_table = s"${table_name}_v"
        val vanilla_create = create(format, Some(vanilla_table))
        vanillaWrite {
          withDestinationTable(vanilla_table, Option(vanilla_create)) {
            checkInsertQuery(insert(format, Some(vanilla_table)), checkNative = false)
          }
        }
        val rowsFromOriginTable =
          spark.sql(s"select * from $vanilla_table").collect()
        val dfFromWriteTable =
          spark.sql(s"select * from $table_name")
        checkAnswer(dfFromWriteTable, rowsFromOriginTable)
      }
    )

    println("hahha")
  }

The left is the content of native written orc file. The right is the content of vanilla written orc file.

Spark version
None
Spark configurations
No response
System information
No response
Relevant logs
No response

The writing process of struct column in orc:

StructColumnWriter::add
IntegerColumnWriter::add


pagination

It would be very useful if jQuery-RSS supported pagination very usefull ,,,

Pull Requests are welcome :)

just u can upgrd it ???   somhow a option  1 to 100 pos or  150-to 190 start limit end limit??

Hi @jayanta119,
It is a nice idea, which I will consider in the future. Not sure when I will find the time to implement that.

any update ???


[Bug Report] Various rendering issues in 'cartpole.py'

Describe the bug

Accepts any string as 'render_mode' then assumes everything else than 'human' to be "rgb_array"
line 512-513: test that 'self.clocks' is None, then sets 'self.clock'...
line 525-526: transposes x but does not pick the to the environment corresponding data

Code example
__init__

        if self.clocks is None:
            self.clock = [pygame.time.Clock() for _ in range(self.num_envs)]

        for state, screen, clock in zip(self.state, self.screens, self.clocks):
            x = self.state.T

System info
Gymnasium installed from source
version 1.0.0rc1
Windows
Python 3.11
Additional context
Issues corrected in the HumanVectorRenderingPR4
Checklist

[X] I have checked that there is no similar issue in the repo

>>> gymnasium.make("CartPole-v1", render_mode="NON_VALID")
/home/master-andreas/gym/Gymnasium/gymnasium/envs/registration.py:735: UserWarning: WARN: The environment is being initialised with render_mode='NON_VALID' that is not in the possible render_modes (['human', 'rgb_array']).
  logger.warn(
<TimeLimit<OrderEnforcing<PassiveEnvChecker<CartPoleEnv<CartPole-v1>>>>>
>>> gymnasium.make("Ant-v5", render_mode="NON_VALID")
/home/master-andreas/gym/Gymnasium/gymnasium/envs/registration.py:735: UserWarning: WARN: The environment is being initialised with render_mode='NON_VALID' that is not in the possible render_modes (['human', 'rgb_array', 'depth_array']).
  logger.warn(
<TimeLimit<OrderEnforcing<PassiveEnvChecker<AntEnv<Ant-v5>>>>>

OK, fixing

BTW noticed
registration.py:722
logger.warn(
"You are trying to use 'human' rendering for an environment that doesn't natively support it. "
"The HumanRendering wrapper is being applied to your environment."
)
Should that really be a warning? Feels like info level to me. Asking for "human", knowing of automatic HumanRendering, getting "human"...

In the case where the environment does not natively support "human" rendering, it should be made clear to the user in case of bug reports

Does 2. and 3.  cause any issues?

Yes,
2. native vector rendering does not work properly
3. vector mode does not work at all

@pseudo-rnd-thoughts
Finally got time to give my commits some time, noticed that the ChartPole and that vector HumanRenderer is also merged.
Note: the ChartPole vector part still contains human renderer (as used in my latest commit).
It should be removed if I understand the discussions here correctly.

@RogerJL Could you clarify where there is a difference in non-vector vs vector CartPole working differently?
See https://github.com/Farama-Foundation/Gymnasium/blob/596bdcc94d068ad494ff4347612df25c6ebec62c/tests/envs/test_env_implementation.py#L259 for the current testing
Yes, we removed "human" rendering intentionally as it is too complex to maintain for each vector environment individually and would prefer if people used the wrappers.vector.HumanRendering instead


Add COG validate helper

Codecov Report

Merging #80 (f57fc6f) into main (237fd9c) will decrease coverage by 0.06%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
- Coverage   83.77%   83.70%   -0.07%
==========================================
  Files          22       23       +1
  Lines        1060     1074      +14
==========================================
+ Hits          888      899      +11
- Misses        172      175       +3


VL training code release?

Reminder

[X] I have searched the Github Discussion and issues and have not found anything similar to this.

Motivation
VL training code release?
Solution
VL training code release?
Alternatives
VL training code release?
Anything Else?
VL training code release?
Are you willing to submit a PR?

[ ] I'm willing to submit a PR!

see #348


GH-242 Add impl for ScriptLanguageExtension::_preferred_file_name_casing

Fixes #242

Opened upstream issue, hopefully this regression can be made optional: https://github.com/godotengine/godot/issues/91569

Applied to main (=2.1).


Add upvotes check to delete_post

Post/comment cannot be deleted if it have valuable upvotes (downvotes are allowed).
It's not required to iterate all votes, check of comment.net_rshares <= 0 will be enough. This can be added with economics part.

#268


2-finger touchpad gesture to zoom the map

Description
Similar to google / leaflet / etc..., I want to zoom the map on my laptop with 2 fingers on the touchpad.  Not having that (or dbl-click) to zoom the map breaks well established ui functionality.
See https://github.com/facebook/RapiD/issues/798

Hi @fpurcell - can you give us more information about what type of system you're using? Macbookpro, thinkpad, etc.? Also, if you use the built-in bug nub (pic:

) that will auto-fill a bunch of information for us like browser version, OS, version of RapiD you're using, etc.

Closing ... on return visit, these gestures now working.
MacBook Air 2020 w/ Ventura 13.2.1
Firefox 109.0.1
Chrome 109.0.5414.119


Grabber refactoring/features

Grabber refactoring overview as notice

[ ] Sync feature level of all grabbers
[ ] Add a monitor selection mode (at .json) (x11) on multi monitor setups to start the grabber always from another source, fallback to monitor 0 if the requested monitor not found
[ ] remove all grabbers from hyperiond (follow hyperion x11)
[ ] connect to kodi check via proto for all grabbers (follow hyperion x11)
[ ] Add to hyperiond a grabber starter which selects automatically the platform depending grabber (after platform check like install script + fallback framebuffer)
[ ] Add enable option to all grabbers - in case of false don´t start the grabber
[ ] v4l2 is a half exception here, cause it is often used at the same .json. But could also be externalized with a opt-in option for kodi check, if people use kodi just via v4l2.

@Paulchen-Panther @redPanther @penfold42
new pattern example - feel free to add more
	"platformgrabber" :
	{
		"enable" : true,
		"width" : 64,
		"height" : 64,
		"frequency_Hz" : 10.0,
		"priority" : 890,
		"display" : 0,
		"cropleft" : 0,
		"cropright" : 0,
		"croptop" : 0,
		"cropbottom" : 0
	},

-> externalize all grabbers this is already done!

of couse, i forgot i rename it to "remove grabbers from hyperiond" ;)

Would be nice if grabbers had a parent class like the led devices. We need a general grabber app too - to reduce code dups

hm, yes makes sense. Could be also used for KodiCheck to disable or enable the grabber completely? (but would be a little bit a log mess, if you start and stop the grabber more frequent)

.. I updated your issue, to be up to date with latest development.

Thanks

More good work!
I guess the next big 'grep' style cleanup we need to do is find all the exit() and throw() calls.
We don't really want any subcomponent stopping hyperiond

exits should be resolved. The throws are ok as long as there catched. We should remove those throws who are in slots, because that is forbidden by QT.
But only removing throws isn't the whole story, we have to adapt the code logic in those areas. If you want to start - v4l would be fine.
problem: I start hyperion, v4l and stuff all is fine. My pc goes to standby, usb grabber is powerd down and bang v4l throws exception because device is not ready/open .... We can do that better!

display selection make not sense on every grabber. e.g. on raspi there is no dual screen posibility. framebuffer has one device per screen, so display selection is selection the correct device.
I suggest, leave the state as it is ....

yes, afterwards i also thought about some of them. Not everything here makes sene, rather work on the effectengine :D

PixelFormat is only available in v4l. Other grabbers didn't support this.
I think we are done here. Of ourse there is always something to do, but we reached a good state in the grabber section ...
@brindosch If you think the same, then close the issue, or drop a comment


Multiple windows, flickering when entering on address bar

OSX

Expected Behavior
Focus should not leave selected window.
Current Behavior

Two windows open
enter a url, press enter, both windows flicker into focus and out for a half a second

Possible Solution
I'd wager this is related to our tab reducer issues. @manavbp FYI

Actually this is same issue as #636

Duplicate of #636, nothing to test, moving to done.


Improve existing log info to include directory paths

eh i guess this would be useful to many new users


make curl settings configurable

The existing defaults for the connection timeout/retries/delay are sometimes insufficient depending on the geolocation of the build environment. Allow these defaults to be overridden.

👍

@carlossg is there anything else you would like me to do to help get this change committed?

Thanks! LGTM, Can you add some description of the options to the README ?


Experiment relationship schema

Can you check that I've got this 'linking table' structure correct and suggest if there might be a better way to do this sort of thing as I think we are going to want to specify the ways Items are related to each other for more than just experiments.
Here is a property in experiment.json:
    "related_experiments": {
            "title": "Related Experiments",
            "description": "Related experiments and how they are related to this experiment.",
            "type": "array",
            "items": {
                "title": "Related experiment",
                "description": "An experiment that is related to this expe....",
                "type": "string",
                "linkTo": "ExperimentRelation"
            }
        },

And here is the experiment_relationship.json:
{
    "title": "Experiment Relation",
    "description": "Schema for capturing relationships.",
    "comment": "Use as mixin.  Do not make stand alone objects",
    "id": "/profiles/experiment_relation.json",
    "$schema": "http://json-schema.org/draft-04/schema#",
    "type": "object",
    "required": ["relationship_type","experiment"],
    "approvalRequired": [],
    "identifyingProperties": ["uuid"],
    "additionalProperties": false,
    "mixinProperties": [
        { "$ref": "mixins.json#/schema_version" },
        { "$ref": "mixins.json#/uuid" },
        { "$ref": "mixins.json#/aliases" },
        { "$ref": "mixins.json#/submitted" },
        { "$ref": "mixins.json#/notes" },
    ],
    "properties": {
        "schema_version": {
            "default": "1"
        },
        "relationship_type": {
            "type": "string",
            "description": "A controlled term specifying the relationship between experiments.",
            "title": "Relationship Type",
            "enum": [
                "controlled by",
                "control for",
                "technical replicate of",
                "biological replicate of",
                "derived from"
            ]
        },
        "experiment": {
          "type": "string",
          "description": "The related experiment",
          "linkTo": "Experiment"
        }
    },
}

Jeremy said that this looks like it'll do the trick.


Issue with k8s.io/docs/setup/production-environment/container-runtimes/

This is a Bug Report

Problem:
CRI-O installation for CENTOS/RHEL7.4+ shows 404 Not Found.

Proposed Solution:
It should work and configure repo on the Centos/RHEL7.4+ system.
Page to Update:
https://kubernetes.io/docs/setup/production-environment/container-runtimes/#cri-o

1.18

/language en

/sig node


Predefine supported generic config keys

Already defined by current code.

:id
filter
sorter

rootdir and builddir. Or perhaps dropping the dir part.

What are they both suppose to define?
rootdir
builddir - the output directory or something like that?

We need definitely decide on what we do for output names.
For markdown I had implemented in my last commits an :outname for each node:

Section :outname:  would produce a subfolder: with an additional option to skip that e.g. for index files with mkdocs.

Page :outname: would result in an actual file name.

I think probaby html might use a similar output structure but pdf not.

What do you think: should we use the :id instead?

:id  seems to be also defined but not set like the rest of configurations id = :myid

What do you think: should we use the :id instead?

Lexicon.Render.get_full_id(::Node) implements this concept. Returns a Vector{Symbol} with the auto generated full path of node ids. Automatically generated names are just based on the node type and position in the children vector, which I feel is clearer than using text from the children.
For markdown output that folder/file structure sounds fine to me. HTML would be similar.

:id seems to be also defined but not set like the rest of configurations id = :myid

That's still being set by any Symbol in the argument list to each node constructor. I fine the syntax quite nice:
document(:custom,
    section(:manual,
        ...
    ),
    section(:reference,
        ...
    ),
)

A keyword id = ... will override a symbol in the argument list.

Related to node ids: I'm thinking that cross-referencing can just be done using
"""
This is a random docstring. This [link](docs/manual/introduction#anchor).
"""

document(:docs,
    section(:manual,
        page(:introduction,
            "page.md",
        )
    )
)

which would link to some anchor in "page.md". Seems like that will match up with your proposed folder structure for markdown/html I think?

What are they both suppose to define?
rootdir
builddir - the output directory or something like that?

Related to node ids: I'm thinking that cross-referencing can just be done using

Yes similar to what I had in mind. the question is an unique anchor which stays always the same even if other filters are applied or the source code is changed and a couple of functions or types are added or removed?
Otherwise it would be quite nice.

rootdir folder to use for resolving external files. Used here.
builddir would be where all save calls send their output so as not to clutter up the working directory. save for different formats could create subdirectories such as markdown, latex, html etc.

the question is an unique anchor which stays always the same even if other filters are applied or the source code is changed and a couple of functions or types are added or removed?

If the target remains on the same page then it should be fine.

builddir would be where all save calls send their output so as not to clutter up the working directory. save for different formats could create subdirectories such as markdown, latex, html etc.

I had previously used :outdir but I think I would leave the dir instead of only root - build

just a question: what about adding one format specific one?
:pdf, :markdown, html, latex

Yes, keeping the "dir" will probably lead to less confusion.

just a question: what about adding one format specific one?

For compile! to make use of?

To have the option to supply a format specific configuration per node.
For instance there should be an option for markdown to disable that the index.md is generated in a subfolder when using mkdocs
I had it here :nosubdir config for such Section.
pdf definately does not need such config option.

For instance there should be an option for markdown to disable that the index.md is generated in a subfolder when using mkdocs

Is that needed prior to actually outputting the markdown files?

If it is then that's fine.

I think it should be per node defineable: and I'm not so sure the configuration passed to  save format Markdown(<>) will allow such - at least not easily.
save(out, Markdown(<<config>>))
save(out, LaTeX(<<config>>))

having one format specifig config option alows that to be a Dictionary in case one needs / wants to store more. (Who knows which formats people might like to add outside of Lexicon?)

Theoretically such format specific config could replace the one which you wanted to pass to save above? - than all settings are in one place - the document.
out = document(
    :one,

    markdown = {.......},
    )

but passing an extra config for format specific general render settings seems also fine.

I figured that separating output config data that's specific to only one format would have the benefit of not needing to change internals of compile! when we add additional formats. Counter examples to that are fine -- If we're not able to easily achieve the same effect by pasing configs to save then I'm not against adding keyword args to the nodes.

In the new Docile (julia 0.4/0.5) not anymore relevant.


minor updates to the readme

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! :smile:

This should re-run the hook.

CLA is valid!

CLA is valid!

CLA is valid!

CLA is valid!


#192 Add consistent invocation of the ensureValidToken callback

This PR fixes the issue: https://github.com/SalesforceCommerceCloud/sfcc-ci/issues/192

Hi @karsov-aiops, thanks for the contribution! Can you please review https://github.com/SalesforceCommerceCloud/sfcc-ci/pull/193/checks? There are test cases failing. Also can you please review https://github.com/SalesforceCommerceCloud/sfcc-ci/pull/193#issuecomment-1413339893? Thanks!


fixing regression introduced in 595e589099ac3d945f53b0a737f590f748ac99e4

Commit 595e589099ac3d945f53b0a737f590f748ac99e4 has introduced a regression where numerous NUL bytes were introduced in the output stream of the decompressed files.
This was because the array slice assignment was not equivalent to the loop item assignments when the array ranges were overlapping. reverting that change fixes the output.
Verified by running the following code:
import mpyq

archive = mpyq.MPQArchive('d2data.mpq') # from a fresh install of Diablo II LoD 1.14d
f = [f for f in archive.files if 'misc.txt' in f.decode('ascii')][0]

with open('misc.txt', 'wb') as o:
    o.write(archive.read_file(f))

I don't remember getting any notification of this one - well it's merged, I guess better late than newer. Thanks.


400 - Template "forms/default/field.html.twig" is not defined in "forms/field.html.twig" at line 1.

FYI
https://demo.getgrav.org/twenty-skeleton/contact

I will take a look at this later.. probably needs an update or removal of forms override.

Noticed this too, exploring the demo. Probably won't happen much in practice with forms plugin enabled.


Improve connection management logic and add login feature

Description:
Rewrite of the connection class to improve functionality and reliability. Commands are now sent sequentially and background tasks are tracked and properly canceled when disconnected. Added option to pass in credential which is used to login to the HEOS devices upon (re)connection. Code improvements to decouple features and logic and move many literal strings to constants. Improved test execution time and other bugs.
Breaking Change:
The constructor of the Heos class has changed to accept an instance of HeosOptions which encapsulates the settings for how a connection to HEOS is maintained. Existing usage will need to update to use the new class.
Related issue (if applicable): fixes #19 and implements ideas from #21
Checklist:

[X] The code change is tested and works locally.
[X] Local tests pass. Your PR cannot be merged unless tests pass
[X] There is no commented out code in this PR.
[X] Tests have been added/updated. No exclusions in .coveragerc permitted.
[X] README.MD updated (if necessary)

Codecov Report
Attention: Patch coverage is 96.13971% with 21 lines in your changes missing coverage. Please review.

Project coverage is 98.59%. Comparing base (97355dc) to head (0b44a04).

Files with missing lines
Patch %
Lines

pyheos/connection.py
93.45%
11 Missing :warning:

pyheos/message.py
93.90%
5 Missing :warning:

pyheos/error.py
81.81%
4 Missing :warning:

pyheos/heos.py
98.96%
1 Missing :warning:

Additional details and impacted files
@@            Coverage Diff             @@
##              dev      #34      +/-   ##
==========================================
- Coverage   99.08%   98.59%   -0.49%
==========================================
  Files          19       19
  Lines        2289     2420     +131
==========================================
+ Hits         2268     2386     +118
- Misses         21       34      +13

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


Use the Python HTTP API from Tauri

We currently communicate from TypeScript <-> Python using a sidecar process. We start a new process whenever we need to communicate. data is sent to Python as a JSON command-line argument and we read JSON back from stdout.
With the web version of RefStudio (#347) we have an HTTP server. This means that the web version and the desktop version have two different ways of communicating. Ideally we'd make the desktop app use the HTTP server as well so that we can unify these code paths.
See @hammer's comment here for some context: https://github.com/refstudio/refstudio/issues/17#issuecomment-1563288836
The simplest path forward is to use Command to spawn the web server and then Tauri's version of fetch to communicate with it.

@danvk great work in #407. Exited to see how we can take advantage of this server and implement a single workflow for the desktop and web.

fixed by #425 and #442


Possible bug with initial sizing of image layout

First of all, I wanted to say that you've done an amazing job with this library; I think it's fantastic. Of the libraries that I've looked into for displaying images in a tiled fashion, this is the one that has met my needs, especially since it displays them in rows (like in Google Photos), which I really like.
I've noticed what might be a bug, so I'll do my best to describe it. Suppose you have a page with a <div id="pig"> to hold the pig layout, as usual when using the library. I've noticed that when the page is first loaded, the image layout as a whole seems to be scaled a little too big, such that it oversteps the right-hand boundary of the div. One way to notice this is to look at a page that has left and right margins (or padding) of equal width on the sides of the content: when the page is first loaded, the right margin will appear a little narrower because the image layout is overstepping the boundary somewhat on the right side. However, if you resize the browser window after the initial page load, everything seems to get sized correctly (and, in this example, the margins would now appear equal).
I was able to observe this phenomenon in Chrome, Firefox, and Safari (all on desktop), both with a site I'm making, as well as with the https://feeding.schlosser.io/ site (which, by the way, is really cool—I love how you documented all that food!).
Are you able to observe this phenomenon as well? I'd love to help out with a fix if I'm able, but since I relatively inexperienced with JS (I've mostly done back-end work in Java thus far), and since I'm not yet very familiar with this library's implementation, I would probably need some pointers along the way.
In any case, thanks again for your great work!

Probably the most notable thing I've found so far is the following. I placed statements to log the width of the pig container (like console.log($('#pig')[0].clientWidth);) before and after new Pig(imageData, options).enable();, and I tried loading the page. I noticed that the container width value before initializing the Pig was larger than the value after, by a difference of 15 (it seems to always be a difference of 15, no matter the browser window width). The smaller value is the "real" one; that is, if you inspect the container element after the page is loaded, its width is the smaller value—and thus the smaller value is also the one that is correct in terms of appearance. This helps explain a proximate cause of the overstepping of the boundary, because it seems that the initial _computeLayout() is done using a container width that's 15 larger than it should be, for whatever reason. Of course, the ultimate cause for the width discrepancy is still a mystery.
I drilled down into enable() to check the values in a finer-grained fashion, like so:
  Pig.prototype.enable = function() {
    console.log('1: ' + $('#pig')[0].clientWidth);
    this.onScroll = this._getOnScroll();
    window.addEventListener('scroll', this.onScroll);
    console.log('2: ' + $('#pig')[0].clientWidth);
    this.onScroll();
    console.log('3: ' + $('#pig')[0].clientWidth);
    this._computeLayout();
    console.log('4: ' + $('#pig')[0].clientWidth);
    this._doLayout();
    console.log('5: ' + $('#pig')[0].clientWidth);
    optimizedResize.add(function() {
      this.lastWindowWidth = window.innerWidth;
      this._computeLayout();
      this._doLayout();
    }.bind(this));
    console.log('6: ' + $('#pig')[0].clientWidth);
    return this;
  };

It appears that the change in width happens during this._doLayout();, which seems reasonable, given that the DOM manipulations happen there. This is what I've found so far, and I hope to drill into the _doLayout() function soon.
To make things more interesting, it seems that this issue does not occur when using mobile Chrome on Android or the mobile simulator in the developer tools in desktop Chrome (those are the only mobile browsers I've tested so far).

Thanks for reporting this issue! Unfortunately, I can't reproduce it on my computer. A few questions:

What OS are you seeing this on?
Can you take a screenshot of this? Or is it only for a second? Can you take a video?

Thanks,
Dan
P.S. Glad you're enjoying the library!!


Feat/add toggle to example app

Add radio buttons to allow user to toggle between trim="input" and trim="blur" in the example app
Move @input() decorator from getter method to setter method in InputTrimDirective to be more in line with common angular design patterns

Awesome! Thank you for the PR! 😇👍


[Parser] Remove remaining -disable-experimental-parser-round-trip uses in test cases

All the features in these test cases are now supported by the new parser, so we don’t need the opt-out anymore.
rdar://124646502

@swift-ci Please smoke test


Add support to indicate the environment or release type of an app

Describe the solution you'd like
A string field to specify the environment of the app. On HockeyApp it's called "Release Type" and can be specified in the app settings.
Describe alternatives you've considered
Another option would be to write it in the name of an app but that's cognitively harder to recognise than having a dedicated field for it.
Additional context
This is important for HockeyApp users which will soon be able to move and still want to differentiate between their app environments.

Would like to see this available as a variable or something we can pull out in our scripts. The typical thing is to have different configurations for each environment but the environment support in App Centre right now doesn't seem to support that. In order to have an app point to a dev, test, and prod endpoint I have to make separate releases or do some funky things with my branches. Any deployment system (Octopus, etc.) has the ability to specify these settings per environment. So if that's what this is, then would like this scriptable so we can make the necessary changes to configuration files based on that environment.

I noticed this has been implemented for the Desktop UI but not mobile. Is there a separate ticket for the mobile UI?

@mkm12789 yes, it has been implemented in the desktop UI only so far. We will be working on the mobile UI in a separate effort when we're integrating the install page with developer portal (FYI @botatoes)


ContentBox Scrolling

anyway to add scrolling inside contentBox class  as style overflow:scroll ?

Is fixxed in Version 2.0.11

Thank you , and amazing cool  UI good job !


show blanknodes

Right now, blanknodes are either not clickable at all or the links are broken.
Map blanknodes into the prefix.

In LodView, blank nodes are embedded into the parent resource, see https://lodview.it/lodview/?IRI=http%3A%2F%2Fhitontology.eu%2Fontology%2FConcreteCompetence&sparql=https%3A%2F%2Fhitontology.eu%2Fsparql&prefix=hito#926ed005d89946e302829bc5844496e8.


WIP: new Spotify API

This list will grow and be kept up to date as development progresses.
This work has been completed in earlier commits:

[x] Update protobufs
[x] Implement HTTP client
[x] Resolve and cache spclient and dealer access points
[x] Implement caching token provider
[x] Add web socket handler
[x] Improve Mercury code legibility

This PR adds the following:

[x] Add HTTPS support
[x] Implement spclient
[x] Migrate metadata from Mercury to spclient
[x] Migrate playlists to playlist4_external proto
[x] Migrate file retrieval to HTTPS CDN
[ ] Publish Connect state on dealer
[ ] Dispatch dealer events
[ ] Implement "repeat single"
[x] Merge dev in (ongoing; last caught up on December 8)
[ ] Test, test, test
[ ] Merge with dev

Optional work
The following work items are somewhat related but optional. They might go in if time allows, or warrant separate PR's later. They don't need to block merging this PR.

[x] Support HTTPS proxies
[x] Fix retrieval of lyrics
[ ] Support playlist editing
[ ] Fix playlist playing state
[ ] Switch lewton to Symphonia to support MP3 and FLAC
[x] Expose additional metadata fields
[ ] Add gzip/deflate support
[ ] Add support for private sessions
[ ] Report played tracks back to Spotify
[ ] Implement context pagination if still applicable on the dealer
[ ] Add documentation
[ ] Implement logout
[ ] Filter explicit content depending on the client configuration
[x] Figure out the autoplay attribute endpoint
[ ] Autoplay depending on the client configuration
[ ] Implement more client configuration knobs (perhaps audio quality, gapless, etc.)
[ ] Implement rate limiting

These things I'm tackling on the go:

[ ] Purge use of unwrap, expect and assert! and return Result
[ ] Return meaningful error types

Finally there is a slew of issues that are targeted to be fixed under the new API.

Running d19fd24 you'll get all metadata and Spotify ID's served over the HTTPS spclient instead of Mercury, except for playlists. Those still seem to be served over Mercury only.
It looks like we can put in the new API piece by piece after all, leaving a working librespot player every time.

I don't know why the build is failing. It compiles just fine here. How is that for you guys?

So obviously 0e26868 is a major rewrite of the metadata crate. I was touching at as I was implementing the new endpoints, getting rid of unwraps and from one thing came the other. Here's a bit more on what's in the commit message:

There were a lot more fields in the protobuf definitions that we never exposed, particularly in recent versions there's been a host of additional data. Now this is all exposed, anyone could use librespot to make a full-featured, information-rich client.

The entire metadata crate is now free of panicking unwraps or expects. Instead it returns Results complete with meaningful error messages. core::SpotifyId has been overhauled to do the same.

The Rust type system is used extensively to convert everything idiomatically with From and TryFrom.

Note that parsing failures in any field can now lead to Err even if the field is optional. This was mostly the case before with unwraps although there were some occasions where parsing was more lenient, e.g. if an album contained a list of tracks and not all of them could be gotten the Spotify ID of, then those were silently discarded and the call returned successfully. It's just that chances of this occurring are greater now, because many more fields are parsed than before.

So far my Spotify sessions have worked fine. But dwelling on this further, hypothetically there could be supporting artists listed (e.g. "feat. Foo Bar") only in text but without Foo Bar having a real Spotify Artist "account" and this would cause the entire album to fail parsing. To circumvent this we could make pretty much everything an Option but this would require a lot of boilerplate everywhere.

So let's test, test, test and first see if this is actually a problem. I mean, everything unwrapped before and it wasn't like we got a lot of panics.

The latest playlist endpoints are in the format of spotify:user:{username}:playlist:{playlist_id} (before, it was just the playlist ID and not the user). This is now supported in addition to the previous, still functional endpoint. For this I also introduced a NamedSpotifyId(pub SpotifyId) that decorates the inner SpotifyId with the username. It's a bit less friendly to use because it can't implement Copy but you can dereference it to the inner SpotifyId in cases you can drop the username.

Endpoints for user root playlists and playlist annotations were added.

There is now an attempt to adhere to embargos (tracks and episodes scheduled for future release). I discovered fields for them in the protobufs but haven't encountered them yet in the wild.

Playlist operation protobufs were implemented for future playlist editing support.

The version number in the handshake was upped to make sure that we're getting all features (Spotify version-gates its features depending on the version of the client connecting).

What I'm going to do next is merge dev in so hopefully many will step in and start using this branch on their own rigs. Then we can weed out all bugs that will undoubtedly come to surface.

if an album contained a list of tracks and not all of them could be gotten the Spotify ID of, then those were silently discarded and the call returned successfully. Not anymore. It's just that chances of this occurring are greater now, because many more fields are parsed than before.

There are many albums that have tracks that are "grayed out" in the official Spotify clients. Meaning they are listed in the UI but not available for playback in Spotify. As in the artist/label/whoever has not made them available on Spotify on purpose.
That did not trip up librespot before that's not going to be a problem now is it?

To circumvent this we could make pretty much everything an Option but this would require a lot of boilerplate everywhere.

I really think that would be the way I'd go for the fields. Actual calls to the API would be a Result because that logically maps to a result but fields should be Option's.

That did not trip up librespot before that's not going to be a problem now is it?

I just merged dev in so you can see for yourself, and report back if it is the case.

I really think that would be the way I'd go for the fields. Actual calls to the API would be a Result because that logically maps to a result but fields should be Option's.

I'd really rather not because wrapping and unwrapping everything would be a pain both for us as lib developers, as well as anyone using that lib. If it turns out to be a necessary evil then I'll do it but lets first see if this is an actual problem.

I'd really rather not because wrapping and unwrapping everything would be a pain both for us as lib developers, as well as anyone using that lib. If it turns out to be a necessary evil then I'll do it but lets first see if this is an actual problem.
I guess that depends on how you implement it. Regardless if the field value is a result or an option I would expect the key to always exist at least.
You wouldn't want something to completely explode if the return value from the Spotify API didn't contain all the expected fields.

You wouldn't want something to completely explode if the return value from the Spotify API didn't contain all the expected fields.

"Expectedness" isn't the problem. Optional fields that aren't in the message are just assigned an empty String or Uuid::nil() or whatever. The problem is any field -- optional or not -- whose value fails parsing. That will cause the entire struct to return Err. The main reason for this would be a missing or malformed Spotify ID.

The problem is any field -- optional or not -- whose value fails parsing. That will cause the entire struct to return Err. The main reason for this would be a missing or malformed Spotify ID.

Then make them results inside options?
struct RandoMetadata {
    someNumber: Option<ParsingResult<u16>>,
    someString: Option<<ParsingResult<String>>,
}

fn get rando_metadata(...) -> MetadataResult<RandoMetadata> {
...
}

struct RandoMetadata {
    someNumber: Option<u16>,
    someString: Option<String>,
}

fn get rando_metadata(...) -> MetadataResult<RandoMetadata> {
...
}

If the API call itself fails then sure return an error. If an individual field does not exist or fails parsing make it None.

That would denote that the fields may not actually exist.

And let the user of our API know that if they do exist but are not of the expected type that there's a problem.

In my example rando_metadata should never actually return an Error unless the actual Spotify API call itself fails.

When it comes to web API's (especially undocumented one's) fault tolerance/robustness trumps ease of use. It does no good to make it easy to use if it explodes if you look at it funny. It's got to be built like a tank.

Like I said I'll do it if and only if it turns out to be a problem.  By all means report it here when you've encountered metadata it couldn't parse.

Like I said I'll do it if and only if it turns out to be a problem.  By all means report it here when you've encountered metadata it couldn't parse.

OK it's your baby but IMHO now is the time to implement it correctly as apposed to hoping things don't fail. It will harder latter if you are mistaken.
It's neither here nor there for me.

In 9a0d239 we pick up on filter-explicit-content mutations that are pushed to us over Mercury. The messages only specify the key to be mutated, but not the new value. This works for filter-explicit-content because we get the initial value after authentication in a ProductInfo packet.
However, this is not the case for the other mutation, autoplay. That initial value doesn't seem to be in the ProductInfo. There is probably a new endpoint to query the Spotify client's settings. If anyone can spend time sniffing traffic and find out, please let me know.
For reference here are two raw protobuf messages:
1 {
  1: "autoplay"
}
2 {
  1:<PHONE_NUMBER>
  2: 593503000
}

1 {
  1: "filter-explicit-content"
}
2 {
  1:<PHONE_NUMBER>
  2: 418909000
}

2.1 is a timestamp from Unix epoch.
2.2 I'm not certain, maybe a connection or device ID of the controlling client, or configuration revision ID.

So by "mutation" you mean change as in change is some client's (controlling device if librespot is used as a Spotify connect "device")? If so those would just be toggles then right? I'm not sure what we'd do in the case of "filter-explicit-content" but toggling the state of "autoplay" is easy enough assuming there's a way to get it's initial state.

If the new API allows librespot to be just a dumb endpoint that inherits most of it's settings from it's controlling device that would be awesome. That would cut the options and flags down by A LOT.

Yes, that's right. Although for now autoplay and filter explicit are the only knobs in the Spotify client settings that trigger a message. And most other settings like bitrate, normalisation and such don't seem to be made available on authentication either.

If you're interested, fire up the latest revision with --verbose and you'll see what's made available on authentication. Most of it will never concern us, but I'm storing it anyway because a downstream player might want to do something with it.

Got it, to receive the initial autoplay value (and some other ones too, like hifi which is clearly in the making) you have to authenticate as PRODUCT_CLIENT and not PRODUCT_LIBRESPOT or PRODUCT_PARTNER like we did before.

If you're interested, fire up the latest revision with --verbose and you'll see what's made available on authentication. Most of it will never concern us, but I'm storing it anyway because a downstream player might want to do something with it.

I will for sure check it out.

Got it, to receive the initial autoplay value (and some other ones too, like hifi which is clearly in the making) you have to authenticate as PRODUCT_CLIENT and not PRODUCT_LIBRESPOT or PRODUCT_PARTNER like we did before.

Nice. If there is a singular source of truth it would be awesome if librespot were basically more or less stateless. For backwards compatibility the various settings could be "forced" if set and stateless if not. Although there's no reason we have to maintain backwards compatibility at all really?

It would be really nice if the output were split up by line and not just dumped as one big object.
But anyway the things that stood out to me by looking are:
audio-quality
enable-crossfade
addon-hifi
hifi-optin-intent
loudness-levels
high-bitrate
enable-gapless
explicit-content

Somethings also seem to be at least slightly redundant like maybe the controlling client is just dumping the kitchen sink as in the output covers different API versions at the same time for backwards compatibility.

Nice. If there is a singular source of truth it would be awesome if librespot were basically more or less stateless. For backwards compatibility the various settings could be "forced" if set and stateless if not. Although there's no reason we have to maintain backwards compatibility at all really?

Unless someone feels otherwise, I intend to remove command line options for things we can get from the client. Certainly backwards compatibility is not at all the goal of this PR, perhaps the contrary.

But anyway the things that stood out to me by looking are:
audio-quality
enable-crossfade
addon-hifi
hifi-optin-intent
loudness-levels
high-bitrate
enable-gapless
explicit-content

At this point only filter-explicit-content and autoplay seem to be pushed when I toggle them from the Spotify client. Look for these messages:
Received attribute mutation, filter-explicit-content was 0 is now 1

The other settings will likely be sent on initial connection.
If you could help, it'd be great if you could try changing the other settings, restart librespot, reconnect to it, and see if and how the initial user attributes are different. If we can find out, I can make as much stateless as possible.

Somethings also seem to be at least slightly redundant like maybe the controlling client is just dumping the kitchen sink as in the output covers different API versions at the same time for backwards compatibility.

Yes that seems to be the case.

It would be really nice if the output were split up by line and not just dumped as one big object.

I think I can do something about that, let me check.

I flipped all the switches in the Spotify Linux Desktop app and the only one that seemed to send anything was explicit-content. Interestingly it seems to be opposite of the switch though. The switch is labeled "Allow playback of explicit-rated content" but it send 0 when on and 1 whe off.

The official Linux app is renowned for being garbage though. It has "libspotify": "1" for example which may mean that it's using the old deprecated libspotify? That would kinda make sense considering you have to use an old version of FFMPEG for local playback to work?

I just checked and my Android phone only sends explicit-content change signals. Most of the settings are greyed out (just like before) and the ones that aren't have no effect including autoplay. Toggling autoplay does nothing.

Just to check we’re talking about the same thing, you’re saying flipping switches then reconnecting makes no further changes to the initial authentication message? My question was not about mutations / update messages.
My Mac client also has libspotify set to 1 while I imagine that Mac is supported as a first-class platform.

Just to check we’re talking about the same thing, you’re saying flipping switches then reconnecting makes no further changes to the initial authentication message? My question was not about mutations / update messages.

No, I didn't check that yet. What I'm saying is that with both the desktop client and on the andriod client the only setting that causes a "mutation" signal is explicit-content.

My Mac client also has libspotify set to 1 while I imagine that Mac is supported as a first-class platform.

All of the desktop apps are the essentially the same thing. They're all built on the Chromium Embedded Framework The Linux version just isn't supported so basically they don't fix Linux specific bugs if ever and it's version lags behind the Windows and Mac versions by sometimes a lot. They're essentially Electron apps. I imagine that the Windows and Mac version's just bundle the crusty old FFMPEG libs by default as is customary for those platforms.
I would not be surprised if they all still use libspotify under the hood.

No, I didn't check that yet. What I'm saying is that with both the desktop client and on the andriod client the only setting that causes a "mutation" signal is explicit-content.

Hmm. I haven't checked from my iPhone app yet, only from my Mac. If it turns out that not all latest versions of the clients support the same options, I don't feel as good about making stateless what we can and removing command line options where we can. Might have to go the route as with the Alsa mixer, like: when you specify a command line option, it's an override, if you don't specify it, it'll take from the client what it can or default to a sane value.

I would not be surprised if they all still use libspotify under the hood.

Sure. All they probably did was pull librespot when they realised they could make money off of licensing. And I don't blame them.

when you specify a command line option, it's an override, if you don't specify it, it'll take from the client what it can or default to a sane value.

That would be my vote. IMHO that's the least inconsistent way to implement. Who knows up other devices like smart home things/services handle things? In that case you're dealing with a controlling program not written by Spotify so who knows what they expose. It's really kinda the wild west. I think robust, resilient and sane wins out over clever.

I'm trying to port my code to this branch as I'd like to experiment with lyrics support.
I'd need convert and decode to be make public again as I'm implementing my own sink, see https://github.com/librespot-org/librespot/pull/814/files
thanks!

I'd need convert and decode to be make public again as I'm implementing my own sink, see https://github.com/librespot-org/librespot/pull/814/files

Oh it's done already in the new-api-wip branch, ignore me. :)

Please welcome the HTTP CDN for audio files in 2f7b986! 🎉
I'm pretty proud of this one because it was quite the journey into Rust async and streaming futures. I'll be touching up small things over the coming days but other than that, this seems to check out!
Please, please, give this some proper testing and report back.

I'm using a modified version of librespot for my Spotty project. I would be willing to test your branch. But as this PR is for a branch which is way behind master or dev, merging is a bit of a nightmare. Wouldn't it make more sense to merge to librespot-org/new-api directly, then create a PR against dev? The way this PR is done now I'm seeing hundreds of modified files which are only about catching up with dev, not modifications in your branch, right?

I fear that merging modified forks won't get much easier no matter which branching workflow we choose. This work has all the characteristics of a major rewrite, and I'm not done yet -- just wait until I get to changing more result types to get rid of panics.
Regularly catching up with dev is the exact thing that's intended to make life easier, at least mine, because this way the merge conflicts with dev are kept to a minimum. I'm not going to put in PR's against dev. The code base has deviated from that way too much.
At this point you can look at current dev like a maintenance branch for 0.3.x. This branch is the one undergoing major development, consider it the future dev.

At this point you can look at current dev like a maintenance branch for 0.3.x. This branch is the one undergoing major development, consider it the future dev.

That is why I have not invested any time in de-panicking any of the network stuff :wink:

On the topic of result error types I welcome opinions on the following:
So far I have been introducing a new error type for every concern, so there's an AudioFileError for an AudioFile, an HttpClientError for an HttpClient, and so on. In order to cross the entire chain from let's say Mercury authentication all the way to file streaming, there's a lot of mapping of one error type to the other. This is exact, but perhaps superfluous especially because so many error variants are quite alike.
To simplify all this, I'm thinking to move to a single error type based on Google's canonical error codes also used in gRPC. So something like a LibrespotError that would cover 99% of concerns, and only introduce more specific error types when truly called for.
Porting https://github.com/grpc/grpc/blob/master/include/grpcpp/impl/codegen/status_code_enum.h this would look something like this:
#[derive(Debug, Error)]
pub enum LibrespotErrorKind {
  #[error("The operation was cancelled by the caller")]
  Cancelled = 1,

  #[error("Unknown error").
  Unknown = 2,

  #[error("Client specified an invalid argument")]
  InvalidArgument = 3,

  #[error("Deadline expired before operation could complete")]
  DeadlineExceeded = 4,

  #[error("Requested entity was not found")]
  NotFound = 5,

  #[error("Attempt to create entity that already exists")]
  AlreadyExists = 6,

  #[error("Permission denied"])
  PermissionDenied = 7,

  #[error("No valid authentication credentials")]
  Unauthenticated = 16,

  #[error("Resource has been exhausted")]
  ResourceExhausted = 8,

  #[error("Invalid state")]
  FailedPrecondition = 9,

  #[error("Operation aborted")]
  Aborted = 10,

  #[error("Operation attempted past the valid range")]
  OutOfRange = 11,

  #[error("Not implemented")]
  Unimplemented = 12,

  #[error("Internal error")]
  Internal = 13,

  #[error("Service unavailable")]
  Unavailable = 14,

  #[error("Unrecoverable data loss or corruption")]
  DataLoss = 15,

  #[error("Operation must not be used")]
  DoNotUse = -1
}

Then wrap and format this with a potential message payload into a LibrespotError like in https://doc.rust-lang.org/stable/src/std/io/error.rs.html.
As always feel free to comment and unless somebody objects, I intend to proceed along these lines.

A user defined /tmp is very likely to fail in modern Linux distro that use systemd because for security purposes users and services user private systemd managed /tmp folders. The idea being that the old school system wide /tmp is a sewer and shouldn't be used. At the very least specifying a /tmp could circumvent the security afforded by such system managed '/tmp` folders.
It would always fail for example in a reasonably secure systemd service like what I designed in Raspotify.
IMHO you're potentially fooling with a system's security implementation details for no real benefit. Where /tmp is can be configured at the system level better than in librespot.

At the very least do not expose the functionality to the librespot binary as it's very likely to get abused by noobs. You know the type "Well it didn't work so I changed everything to 777 and run it as root."

I disagree. Other than the obvious arguments "me and many others don't run from systemd", "it's optional" and "only so much you can do to prevent others from shooting into their own foot", this really is about reliability and not so much about security.
The temporary files are AES-encrypted and the librespot process holds the key in memory. So from a security perspective it wouldn't really matter if other users could access the temporary files.
What this is about, is reliability in the face of pathological temporary file cleaners. We use NamedTempFile that is susceptible to such cleaning. Yet we need to use that instead of tempfile, because NamedTempFile allows us to create and write to a new file, then directly reopening it for reading at the same time for streaming. tempfile isn't susceptible to cleaners, but can't guarantee that it'd reopen the same file.
The documenation I linked to provides a decent description and trade-off of what's going on.
Please disregard c758aea, I'll force-push what I intended later.

So essentially you're guarding against one misconfiguration with another. Seems like an antipattern.

We already use a cache I'm not sure what you're trying to accomplish some kind of disk backed download buffering? If so you are going to kill a lot of microSD cards. Are you afraid that a track won't fit in memory? I don't really see the point of tmp files and a cache. Also with tmp files in non standard places the responsibly now becomes yours to make sure you are cleaning up after yourself.

Unfortunately, most Linux distributions don't create per-user temporary file directories. Worse, systemd's tmpfiles daemon (a common temporary file cleaner) will happily remove open temporary files if they haven't been modified within the last 10 days.

Not an issue:

The vast majority of Linux distros do in fact use systemd.
There is no reason at all for us to have a 10 day old tmp file so that's a non-issue.

If the program exits before the NamedTempFile destructor is run, such as via std::process::exit(), by segfaulting, or by receiving a signal like SIGINT, then the temporary file will not be deleted.

This is the gotcha. If you're not very careful you will fill up someone's disk with garbage tmp files. Since you put them in a non-standard place if you do not delete them no one will.
Your solution is potentially more damaging then the problem it's trying to solve that more than likely does not exist.

So essentially you're guarding against one misconfiguration with another. Seems like an antipattern.

I think user temporary directories instead of global temporary directories are a great idea. I think systemd private tmp directories support that. I also think that not doing the right thing because your users may choose to chmod 777 and hit enter is an anti-pattern. So let's not dwell on this too much. If you don't like it, then don't use it. Do nothing and it will default to what it has always done.

We already use a cache I'm not sure what you're trying to accomplish some kind of disk backed download buffering? If so you are going to kill a lot of microSD cards. Are you afraid that a track won't fit in memory? I don't really see the point of tmp files and a cache.

Relax. This is how it's been in librespot for ages. I think it's a a good idea to use RAM-backed storage. For lossy audio at least, I don't know about FLAC. It's not so easy to switch or offer the option though, because everything in librespot up to the decoders expect fs::File behaviour. https://crates.io/crates/rsfs looks nice but doesn't work on Windows.
But let's first recognise that this is nothing new and a separate issue and potential PR altogether.

Also with tmp files in non standard places the responsibly now becomes yours to make sure you are cleaning up after yourself.

Or the user who himself explicitly specifies a non-standard place. Which is entirely common with packages that offer setting up customised tmp locations.

Your solution is potentially more damaging then the problem it's trying to solve that more than likely does not exist.

Please stop. I offered something you can opt-in to. Nothing more.

I think user temporary directories instead of global temporary directories are a great idea. I think systemd private tmp directories support that.

That's standard.

I also think that not doing the right thing because your users may choose to chmod 777 and hit enter is an anti-pattern. So let's not dwell on this too much. If you don't like it, then don't use it. Do nothing and it will default to what it has always done.

I have no problem with exposing the feature in the libs for devs but It's a bad idea IMHO to expose the functionality to users of the librespot binary.

Relax. This is how it's been in librespot for ages. I think it's a a good idea to use RAM-backed storage. For lossy audio at least, I don't know about FLAC. It's not so easy to switch or offer the option though, because everything in librespot up to the decoders expect fs::File behaviour. https://crates.io/crates/rsfs looks nice but doesn't work on Windows.

No. /tmp was not configurable before.

Or the user who himself explicitly specifies a non-standard place. Which is entirely common with packages that offer setting up customised tmp locations.

Fully loaded foot guns for noob users. I guess you're the one that has to deal with the noise.

Please stop. I offered something you can opt-in to. Nothing more.

There are already to many options and flags. I'm not a fan of adding more especially when they're full of so many gotchas.

Relax. This is how it's been in librespot for ages. I think it's a a good idea to use RAM-backed storage. For lossy audio at least, I don't know about FLAC. It's not so easy to switch or offer the option though, because everything in librespot up to the decoders expect fs::File behaviour. https://crates.io/crates/rsfs looks nice but doesn't work on Windows.

No. /tmp was not configurable before.

Please don't take my words out of context.

Please don't take my words out of context.

I think you're trying to solve a problem that does not exist and in the process giving users the ability shot themselves in the foot. That's just MHO, but I'm done. You can and will do what you want.

It's not so easy to switch or offer the option though, because everything in librespot up to the decoders expect fs::File behaviour. https://crates.io/crates/rsfs looks nice but doesn't work on Windows.

At 1st glance it doesn't look horribly complicated to create a "file like object" in rust:
https://stackoverflow.com/questions/41069865/how-to-create-an-in-memory-object-that-can-be-used-as-a-reader-writer-or-seek

That's certainly worth investigating.

I bit the bullet and have been working on removing all the calls to unwrap and expect for days, in all crates except playback. Meanwhile refactoring to a ubiquitous librespot_core::Error type. While not the most exciting work, I am glad that I did because it's a lot of work, but would be even more so if we first added new features and then started looking back. This is going to be a huge commit touch nearly every file.
While at it, is anyone opposed to introducing parking_lot to replace the currently used Mutex, RwLock and Condvar from std? The advantage being that the ones from parking_lot can't get poisoned so calls to lock don't require unwrapping and can't panic.

So this was my own take on Spotify Un-Wrapped 😜
Merry Christmas everyone.

@JasonLG1979 I left purging unwrap and expect from playback for a later time. If you'd be willing to take that up, feel free to submit a PR to my branch.

@JasonLG1979 I left purging unwrap and expect from playback for a later time. If you'd be willing to take that up, feel free to submit a PR to my branch.

git/GitHub really isn't built for that kind of workflow. I can't contribute directly your fork easily because what I'd be creating is a fork of a fork and git wants me to contribute to librespot proper. I can work on playback as soon as you merge this into at least the new-api branch in librespot proper. At some point you should really start to merge this into 'new-api` to make it possible for other people to contribute.

@JasonLG1979 I left purging unwrap and expect from playback for a later time. If you'd be willing to take that up, feel free to submit a PR to my branch.

git/GitHub really isn't built for that kind of workflow. I can't contribute directly your fork easily because what I'd be creating is a fork of a fork and git wants me to contribute to librespot proper. I can work on playback as soon as you merge this into at least the new-api branch in librespot proper. At some point you should really start to merge this to make it possible for other people to contribute.

Sure you can. You can do git remote add on your existing clone, or clone anew. Then submit a PR to my repository and when I merge it, it should show up here too.

Sure you can. You can do git remote add on your existing clone, or clone anew. Then submit a PR to my repository and when I merge it, it should show up here too.

Like I said extra steps and not really the normal work flow. The normal workflow would be for everyone to contribute from their forks into librespot proper and collaborate there not in someone's fork of librespot.

You have commit rights to this repo I'm not sure why you have a personal fork? You can just create your own branch here and merge it into the new-api branch when you wanted to. That would also make for a more logical and standard workflow for people who wanted to help you.

We started doing this when it was the intention that both @Johannesd3 and myself, and possibly others, would develop new-api in parallel. I guess it stuck. With the advantage that we can have some discussion here.

We started doing this when it was the intention that both @Johannesd3 and myself, and possibly others, would develop new-api in parallel. I guess it stuck. With the advantage that we can have some discussion here.

That's fine but you need to merge your work into new-api every now and then. Alternatively if you want to track and discuss things you can start a new-api-wip branch that has an open PR to the new-api branch and accomplish the same thing and make the workflow for everyone else more logical.

We started doing this when it was the intention that both @Johannesd3 and myself, and possibly others, would develop new-api in parallel. I guess it stuck. With the advantage that we can have some discussion here.

That's fine but you need to merge your work into new-api every now and then. Alternatively if you want to track and discuss things you can start a new-api-wip branch that has an open PR to the new-api branch and accomplish the same thing and make the workflow for everyone else more logical.

That's a good idea. I was thinking of doing an intermediate merge after ironing out a few wrinkles.

We started doing this when it was the intention that both @Johannesd3 and myself, and possibly others, would develop new-api in parallel. I guess it stuck. With the advantage that we can have some discussion here.

That's fine but you need to merge your work into new-api every now and then. Alternatively if you want to track and discuss things you can start a new-api-wip branch that has an open PR to the new-api branch and accomplish the same thing and make the workflow for everyone else more logical.

That's a good idea. I was thinking of doing an intermediate merge after ironing out a few wrinkles.

Cool. You can still rapidly commit to whatever branch. It's not hard to rebase when it's incremental changes the hard part is when you diverge so much like this PR. Just merge this as-is right now so we can all help. Unless you like doing all the work?

OK so I get this right in one time, what I intend to do is:

merge the current state of this PR into new-api
create a discussion to track my progress
commit changes directly to new-api
review and merge your PR's onto new-api

I think that branching off another new-api-wip branch and creating a pull request off that would bring us in the same situation we are in now. We don't need a nightly (new-api-wip) -> beta (new-api) workflow. Let's just all jump onto the new-api train and allow me to reap the benefits of my commit bits.

You might want to catch up new-api with dev. You didn't rebase before you merged.

Let's continue in #917.


fix: Use freeze's built-in line selection to pass lines to freeze.

freeze already has support for passing what lines you want to capture in a file, that should be how we handle this rather than passing it in via stdin.

Fixed in most recent commit. Closing.


Managed Package Publication

Summary:
This document proposes publishing the Trigger Actions Framework (TAF) as a managed package. This will allow other packages, including those from Independent Software Vendors (ISVs), to formally establish a dependency on TAF.
Benefits:

Increased adoption: Publishing as a managed package will make TAF more discoverable and easier for both customers and ISVs to integrate into their Salesforce solutions.
Improved developer experience: ISVs can build on top of TAF without needing to embed the code directly in their packages. This simplifies development and maintenance.
Enhanced collaboration: A managed package fosters a more collaborative ecosystem around TAF.

Considerations for Managed Package Publication:

Namespace: Introduce a namespace field on the Trigger_Action__mdt custom metadata type to ensure code isolation and prevent conflicts with other packages.
Global vs. Namespaced: Carefully review all classes and interfaces to determine if they should be global (accessible across all packages) or namespaced (accessible only within the TAF package). Consider the intended use of each class/interface and potential conflicts.
Backwards Compatibility: Develop a plan to maintain backwards compatibility with existing customers who use the unlocked package. This may involve versioning and providing clear migration paths for future updates.
Documentation: Update documentation to reflect changes introduced for the managed package version.

Next Steps:

Review Salesforce documentation on managed packages and best practices.
Finalize the design for namespace implementation and global vs. namespaced classes/interfaces.
Develop a backward compatibility plan for existing users.
Prepare documentation for the managed package.

@mitchspano here are a couple of considerations/things I've run into when doing this same thing for Nebula Logger

For fields on CMDT objects, you'll want to decide what makes sense to use for fieldManageability (options are DeveloperControlled and SubscriberControlled)

I think using SubscriberControlled would make the most sense for TAF, but not 100% sure about that

If you add your namespace to your sfdx-project.json file, then only people with access to your devhub (presumably, that's only you?) will be able to (easily) work with the repo - if anyone else tries to create a scratch org using your repo, they'll get an error because they don't have access to whatever namespace you use

@jamessimone and I have taken the approach of having a second sfdx-project.json file (in Nebula Logger and Apex Rollup), and our pipelines swap out sfdx-project.json files when building the namespaced/managed package versions. It's a bit goofy & frustrating, but you can setup your pipeline to automatically handle it for you

By default, you can't deprecate/delete any metadata from a managed package - if/when you think you'll need to do that, you'll have to get Salesforce to enable it in your devhub

I'm sure there's some other stuff I'm forgetting - I'll let ya know if I think of anything else!

Following up on @daveespo's comment - even if you get TAF security reviewed & published in AppExchange, some ISVs still may not want to directly rely on another package (TAF), so some ISVs may still take TAF's metadata & bundle it into their own package. There are a handful of ISVs that do this with Nebula Logger. But by providing TAF as a managed package, you'll have the metadata in a state that makes it easy for ISVs to pull it into their own packages (if they want to go with that approach).

A similar discussion was held in the past about fflib here / here.


fix rn 615

last pr was missing react native updating, just react , sigh

Does this need a pod install?

yeah, one sec

actually lemme fix upsome other stuff

@jakob223 @songgao @buoyad ok fixed some gradle/pod stuff


Blog search doesn't work

Search on blog.rasa.com doesn't work.

@rgstephens Do we have code residing in this repo? I searched for blog source couldn't find one. Should this be fixed here?

Closing as the search seems to have been removed from the blog website


AEBN Matching issue

Working with the title Freaky Chicken at http://gay.theater.aebn.net/dispatcher/movieDetail?genreId=102&theaterId=14077&movieId=164809&locale=en
There appear to be at least two titles with "Freaky Chicken."  I've tried "Vimpex - Freaky Chicken.ext" "Vimpex Freaky Chicken.ext" "Streetdancers - Freaky Chicken.ext" "Streetdancers Freaky Chicken.ext" and it won't match.  Looked in the logs and not really seeing a lot since the AEBN agent seems to not log as much information as Helix or Staxus.

I'll look into this latter. Add the studio name in front. e.g. "(staxus) - title name". Also make sure the logs are turned on for AEBN.

Okay, looks like I needed to do the full title of Vimpex Gay Media with () to make this one match.

I tried your Solution with the () in front of the title including the studio name but it still does not match. I try to match (Mustang) - Man Up but with no luck. it matches the first result of Man Up.
Any solution?

@vittsof can you provide the log file for debuging

com.plexapp.agents.aebn.txt
Thank you for the response.

Okay I found the issue and will try and push the fix today. I accidently added a bug into the code that prevented it from working at all. Version 2<IP_ADDRESS> will work correctly.

Thank you Very much!!! I appreciate it!

@vittsof I believe the update fixed the issue. Let me know if it continues. I only tested it with one title I had.

@MrPlow254 So far its working just fine. Thank you for your Help!


Maybe a bug,rancher/server container broken and could not restart

Rancher versions:
rancher/server:1.3 and 1.6 both
1.6 for mysql HA
1.3 for run container directly
i don't known how to do, it just happen one day
docker logs --tail  containerId,any it say:
./run not found.
so i dock exec -it into the container,i found that the /service has changed
Normally, the folder structure is:
/service
    |
   cattle/{event/,supervise/,run,finish}
    |
    |
   mysql/

when broken, it come to be:
/service
   |
   cattle/
   |
   mysql/
   |
   event/
   |
   supervise/
   |
   run
   |
   finish

Maybe the folders of  [event & supervise] in cattle before were all moved out.
So it repaired the forders, it could be restart and work well again.

Not really sure what happened in this case, if you have a way to reproduce on the latest version, let me know.


fixes for freebsd cron invocation and kitchen changes required to test

the cron recipe would not converge under freebsd, there's no init.d equiv and I thought the cleanest way would be to put a pair of not_if guards on the two unneeded templates. I also had to add the '"/bin/sh"' to the .kitchen.yml to get kitchen to work at all with the freebsd box files. I tested that the freebsd/cron suite works under vagrant and that my '"/bin/sh"' changes do not break at least the ubuntu suites.

I note that travis is and has been failing but PR #337 fixes that issue. My local kitchen tests succeeded, but it would be nice to get the green travis badge of happiness as well.

lol, confusing...

let's combine our lamont powers for great justice!  But on a more serious note, I'm happy to yank out my .travis.yml changes if the other PR does a better job. the .kitchen.yml change was required to get freebsd working under kitchen.

I don't have a lot of opinion on rake spec vs rake travis, it should probably bundle exec either way for less confusion.

Ok, I'll rework the .kitchen.yml change so that the ssh/shell is applied to the freebsd platform specifically and put the two templates in an anti-freebsd block then ping you again. Probably Thursday or Friday.  And I'll add the .foodcritic ignore for the warning I'm introducing.

Well, that fixes rake spec on my machine, but the TravisCI build fails cause we hit the 4 megabyte log limit due to the repeated warnings about needing override: true in new resources.
WARN: You declared a new resource #Class:0x007fbd95c3d430 for resource windows_certificate, but it comes alphabetically after LWRP resource windows_certificate from cookbook windows and has the same filters ({}), so it will not be used. Use override: true if you want to use it for windows_certificate.
I have no idea why that warning suddenly appeared, but it's affected other tests I was running using the apt cookbook. Happened under chefDK 0.7.0 and also 0.8.1.

@lamont did this ever get complete to your satisfaction?

looks fine to me but looks like it needs a serious rebase.

@lamont Please check ^

I'm going to close this one out at this point due to the age and drift in the codebase. If this is still something you want to do let me know in the community slack and we can figure out next steps.


storing value of pointer to a pointer segfaults

x = 1

pointer = pointerof(x)

p pointer

p pointerof(pointer)

This works perfectly fine, but if I try to store value of pointer to pointer it segfaults.
x = 1

pointer = pointerof(x)

p pointer

x =  pointerof(pointer)

Error
$ crystal pointer.cr
/usr/local/bin/crystal: line 6: 77831 Segmentation fault: 11  "$INSTALL_DIR/embedded/bin/crystal" "$@"

It segfaults the compiler, looks like a stack overflow (32k entries in the backtrace).
#28437 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28438 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28439 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28440 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28441 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28442 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28443 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28444 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28445 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28446 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28447 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28448 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28449 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28450 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28451 0x0000555555b79208 in *Crystal::ASTNode+@Crystal::ASTNode#notify_observers<Crystal::ASTNode+>:Array(Crystal::Call)? ()
#28452 0x0000555555b78ae2 in *Crystal::ASTNode+@Crystal::ASTNode#propagate<Crystal::ASTNode+>:Array(Crystal::Call)? ()

The issue does not occur if we replace x by another variable in the last line:
x = 1
pointer = pointerof(x)
p pointer
y = pointerof(pointer)  # works
p y

The issue is caused by overwriting the x variable with a pointer to pointer to itself. In that case it is impossible to derive a correct type union for x, as the union is infinitely recursive:
x :: Int32 | Pointer(Pointer(Int32))  # won't compile, Pointer(Pointer(Int32)) is not a pointer to pointer to itself

This is how it would look...
x :: Int32 | Pointer(Pointer(Int32 | Pointer(Pointer(Int32 | Pointer(Pointer(Int32 | ...

@pgkos yes you're right .
If you do this :
x = 1
x = pointerof(x)

it throws this error :

$ crystal first.cr
Error in ./first.cr:40: recursive pointerof expansion: Pointer(Int32 | Pointer(Int32 | Pointer(Int32 | Pointer(Int32 | Pointer(Int32))))), Pointer(Int32 | Pointer(Int32 | Pointer(Int32 | Pointer(Int32 | Pointer(Int32 | Pointer(Int32)))))), ...

While this code segfaults
x = 1
pointer = pointerof(x)
x =  pointerof(pointer)

this throws  an error
x = 1

x = pointerof(pointerof(x))

$ crystal pointer.cr
Error in ./pointer.cr:40: can't take address of pointerof(x)

x = pointerof(pointerof(x))


Free unused memory in Data.from

At JPG Store, we have been dealing with memory leaks in our code that uses either Lucid or the CML. After some research, we discovered that the CML does a lot of clones to avoid issues with freed memory across the WASM boundary ( https://github.com/dcSpark/cardano-multiplatform-lib/issues/142#issuecomment-1281810732).
Any object that is returned from WASM to JavaScript seems to never get garbage collected. Thus, all objects returned from wasm to the js runtime must be freed, including intermediary values when chaining function calls together.
This PR specifically only fixes  the memory usage in the Data.from function to resolve the following issue: https://github.com/spacebudz/lucid/issues/222
I will continue to fix the memory management over time, as I have free time. However, I would argue it is best to merge these smaller pieces whenever they are done, as they will reduce the memory footprint of several applications relying on Lucid.

Closing in favor of https://github.com/spacebudz/lucid/pull/234 which includes managing WASM memory everywhere it's used.