Advanced Transactions

Previously Transactions were limited to FilterRules only and wouldn't allow to change rules across multiple anchors. Well, not anymore!
This PR adds support for multi-rule and multi-anchor updates by introducing AnchorChange that represents the manipulation over anchor that will happen during Transaction.
AnchorChange has Optional arrays of FilterRules and RedirectRules, you may wonder why. Well the reason for that is ability to perform targeted updates, for example: one may decide to update filter rules but leave redirect rules untouched. Running empty transaction would erase the list of rules for corresponding anchor and ruleset kind pair, that's why we should not obtain tickets for rulesets that we do not plan to change.
Transaction itself now accepts array of AnchorChanges that enables consumers to update multiple anchors in one go.
It's worth to notice that changes are grouped by anchor and adding AnchorChanges for the same anchor twice would only substitute the former value.
Integration tests to follow.

This change is

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

src/transaction.rs, line 40 at r1 (raw file):

    /// if any.
    pub fn add_change(&mut self, anchor_change: AnchorChange) {
        if let Some(index) =

Since transactions replace rules we cannot add two changes for the same anchor. We could throw error instead of replacing the former change if that's a better behavior.

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

src/transaction.rs, line 23 at r1 (raw file):

pub struct Transaction {
    file: File,
    anchor_changes: Vec<AnchorChange>,

I used HashMap<anchor_name: String, AnchorChange> in the beginning but later dropped it because it seemed like preliminary optimization.

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

src/transaction.rs, line 40 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Since transactions replace rules we cannot add two changes for the same anchor. We could throw error instead of replacing the former change if that's a better behavior.

Also, I could put back the HashMap, this should probably be more optimal then.

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 219 at r1 (raw file):

    pub struct TransactionElements<'a> {
        heap: Vec<ffi::pfvar::pfioc_trans_pfioc_trans_e>,
        filter_indices: Vec<(&'a AnchorChange, usize)>,

The reason why AnchorChange is hanging all over is because I can then associate it with certain transaction and ticket.

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 23 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I used HashMap<anchor_name: String, AnchorChange> in the beginning but later dropped it because it seemed like preliminary optimization.

I switched it back to HashMap.

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

src/transaction.rs, line 40 at r1 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Also, I could put back the HashMap, this should probably be more optimal then.

Replaced with HashMap, easier to update/replace the anchor

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 259 at r2 (raw file):

        pub fn get_redirect_elements
            (&'a self)
             -> Vec<(&'a AnchorChange, &'a ffi::pfvar::pfioc_trans_pfioc_trans_e)> {

It's unnecessary to return pfioc_trans_e from here. The only information used outside is pfioc_trans_e.ticket.

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

src/transaction.rs, line 215 at r2 (raw file):

        heap: Vec<ffi::pfvar::pfioc_trans_pfioc_trans_e>,
        filter_indices: Vec<(&'a AnchorChange, usize)>,
        redirect_indices: Vec<(&'a AnchorChange, usize)>,

Indices for filter and redirect rules exist in order to navigate in the heap of pfioc_trans_e elements cruising between PF and pfctl-rs and be able to tell filter tickets from redirect tickets.
Since we avoid working with FFI data we rely on pre-built indices to tell us what is what.

Comments from Reviewable

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions.

src/transaction.rs, line 235 at r2 (raw file):

        /// Registers transaction elements for each ruleset in AnchorChange
        pub fn add_elements(&mut self, anchor_change: &'a AnchorChange) -> Result<()> {
            if anchor_change.filter_rules.is_some() {

Since transactions replace rule, requesting ticket is the same as flushing it. AnchorChange uses Option<Vec> to express that.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions.

src/lib.rs, line 288 at r3 (raw file):

    }

    pub fn flush_rules(&mut self, anchor: &str, kind: RulesetKind) -> Result<()> {

We should accept Set<RulesetKind> in future to flush redirect + filter rules in one go!

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 7 unresolved discussions.

tests/filter_rules.rs, line 186 at r4 (raw file):

        .unwrap();
    for rule in [rule1, rule2, rule3, rule4].iter() {
        assert_matches!(pf.add_rule(ANCHOR_NAME, rule), Ok(()));

can just .unwrap()

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions.

tests/transaction.rs, line 89 at r5 (raw file):

}

fn assert_eq_filter_rules(pf_rules: Vec<String>) {

Annoying bunch of helpers, but they are used more than 2-3 times each so that's why I have them

Comments from Reviewable

Reviewed 1 of 5 files at r1.
Review status: 1 of 6 files reviewed at latest revision, 9 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):

#[derive(Debug)]
pub struct AnchorChange {
    pub anchor: String,

It feels like the anchor name does not belong inside this struct. Feels better to give that to the method on Transaction where the AnchorChange is added. It's not used inside the struct, is it?
Having it inside the structs creates more hard-to-follow indirection IMO. Because Transaction will have to pick it out, but the user will never see it. Imagine the following scenario:
let transaction = Transaction::new();

let change_a = calculate_changes_a();
let change_b = calculate_changes_b();

transactions.add_change(change_a);
transactions.add_change(change_b);

In the code above it's impossible to know which anchors they touch, or if they are even the same anchor. So it can lead to hard-to-find bugs. Better to do like HashMap, where the key is always given on insert.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 9 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
It feels like the anchor name does not belong inside this struct. Feels better to give that to the method on Transaction where the AnchorChange is added. It's not used inside the struct, is it?
Having it inside the structs creates more hard-to-follow indirection IMO. Because Transaction will have to pick it out, but the user will never see it. Imagine the following scenario:
let transaction = Transaction::new();

let change_a = calculate_changes_a();
let change_b = calculate_changes_b();

transactions.add_change(change_a);
transactions.add_change(change_b);

In the code above it's impossible to know which anchors they touch, or if they are even the same anchor. So it can lead to hard-to-find bugs. Better to do like HashMap, where the key is always given on insert.

If an AnchorChange is supposed to represent a set of rules to apply to an anchor, then which anchor is not a property of that. It would likely be cleaner to make it represent only the new rules, and then the user decides where to apply those rules.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 9 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
If an AnchorChange is supposed to represent a set of rules to apply to an anchor, then which anchor is not a property of that. It would likely be cleaner to make it represent only the new rules, and then the user decides where to apply those rules.

Yeah but I use anchor when I add rules from each of changes, you don't want me to pick up anchor from pfioc_trans_e do you? I am trying to rework that to get rid of TransactionElements if I will be able to achieve that.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 9 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Yeah but I use anchor when I add rules from each of changes, you don't want me to pick up anchor from pfioc_trans_e do you? I am trying to rework that to get rid of TransactionElements if I will be able to achieve that.

You proposal does not cover lib.rs case when we need to pass an array of changes for multiple anchors, so either tuples or some wrapper struct would work

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 9 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
You proposal does not cover lib.rs case when we need to pass an array of changes for multiple anchors, so either tuples or some wrapper struct would work

Then the question is if we really need to pass multiple AnchorChange at once? Do you mean pub fn add_changes(&mut self, change_by_anchor: Vec<AnchorChange>)? According to the docs it was added just for convenience.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 9 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Then the question is if we really need to pass multiple AnchorChange at once? Do you mean pub fn add_changes(&mut self, change_by_anchor: Vec<AnchorChange>)? According to the docs it was added just for convenience.

I mean set_rules in lib.rs

Comments from Reviewable

@pronebird I really love that you removed the part that used indices to jump around in ffi structs. This is so much better IMO!

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I mean set_rules in lib.rs

I feel that set_rules in lib.rs is also mostly a convenience method. It would be easy to do what it does directly with the Transaction struct. So I still suggest moving the name of the anchor out of the struct and make the methods adding them take the anchor as the first argument.
If one want to use the convenience method PfCtl::set_rules(anchor: &str, change: AnchorChange) they will be limited to setting one anchor. But I don't see that as a problem, for those wanting to do more complex operations they should probably just use Transaction directly anyway.

src/transaction.rs, line 32 at r6 (raw file):

        Ok(
            Transaction {
                file: utils::open_pf()?,

I guess it would be possible to open the file in commit, to make the constructor infallible. Catching all errors in commit instead. What do you think about that? Would simplify error handling for users of the library, they probably just wrap both these errors up in the same parent error anyway, so we can spare them the hassle. Opening the file here already is more of an artificial limitation than an actual requirement if I didn't misunderstand.

src/transaction.rs, line 85 at r6 (raw file):

                        &c.anchor,
                        c.filter_rules.as_ref().unwrap(),
                        element_iter.next().unwrap().ticket,

A nicer way to iterate two iterators at the same time, that does not involve unwrap() would be to use Iterator::zip() to zip two iterators together.

src/transaction.rs, line 89 at r6 (raw file):

                },
            )
            .collect::<Result<Vec<_>>>()?;

This iter + map + collect approach is IMO quite hard to read. Why not just a normal for loop?

src/transaction.rs, line 144 at r6 (raw file):

        // add rule into transaction
        ioctl_guard!(ffi::pf_add_rule(self.fd(), &mut pfioc_rule))
            .chain_err(|| "pf_add_rule failed")

Can you change this into a real error? Just not chaining it and thus let it return the IoctlError on failure sounds reasonable. Same goes for add_redirect_rule.

src/transaction.rs, line 148 at r6 (raw file):

    /// Internal function to add a batch of filter rules into transaction
    fn add_filter_rules(&self, anchor: &str, rules: &[FilterRule], ticket: u32) -> Result<()> {

I kept scrolling up and down very much in this file. It would be nice if it could adhere to the "stepdown rule" a little bit. Since add_filter_rules and add_redirect_rules is what is being used in commit I'm going to scroll between those very often, so good to have them directly after commit, and not cram in add_filter_rule in between. Then the methods those methods use are below them etc. Lowest level methods at the bottom.
For some code this rule is hard to apply, but here I think it makes lots of sense, it's a pretty clear tree of dependent functions: commit -> add_filter_rules -> add_filter_rule etc.

src/transaction.rs, line 153 at r6 (raw file):

            .map(|rule| self.add_filter_rule(anchor, rule, ticket))
            .collect::<Result<Vec<_>>>()
            .map(|_| ())

Just doing an ordinary for loop here instead of iter + map + collect would actually be less lines of code, and would IMO be much more straight forward to read. Compare the above with:
for rule in rules.iter() {
    self.add_filter_rule(anchor, rule, ticket))?;
}
Ok(())

Comments from Reviewable

@faern thanks Linus! Took me a while to figure out that I could do that differently ...

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I feel that set_rules in lib.rs is also mostly a convenience method. It would be easy to do what it does directly with the Transaction struct. So I still suggest moving the name of the anchor out of the struct and make the methods adding them take the anchor as the first argument.
If one want to use the convenience method PfCtl::set_rules(anchor: &str, change: AnchorChange) they will be limited to setting one anchor. But I don't see that as a problem, for those wanting to do more complex operations they should probably just use Transaction directly anyway.

Agree. So basically we'll have to open Transaction and open_pf() to consumer.

src/transaction.rs, line 153 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Just doing an ordinary for loop here instead of iter + map + collect would actually be less lines of code, and would IMO be much more straight forward to read. Compare the above with:
for rule in rules.iter() {
    self.add_filter_rule(anchor, rule, ticket))?;
}
Ok(())

Done.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 32 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I guess it would be possible to open the file in commit, to make the constructor infallible. Catching all errors in commit instead. What do you think about that? Would simplify error handling for users of the library, they probably just wrap both these errors up in the same parent error anyway, so we can spare them the hassle. Opening the file here already is more of an artificial limitation than an actual requirement if I didn't misunderstand.

You're right, we should open PF on demand, there is no good reason to keep it alive. The only thing is, we'll have to pass File or RawFd around.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 85 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
A nicer way to iterate two iterators at the same time, that does not involve unwrap() would be to use Iterator::zip() to zip two iterators together.

That would work, but for redirect_rules we would have to skip() first N elements.

src/transaction.rs, line 89 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
This iter + map + collect approach is IMO quite hard to read. Why not just a normal for loop?

Agree. Changing this.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 85 at r6 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
That would work, but for redirect_rules we would have to skip() first N elements.

Actually, I can share iterator :) no need to skip

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 148 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I kept scrolling up and down very much in this file. It would be nice if it could adhere to the "stepdown rule" a little bit. Since add_filter_rules and add_redirect_rules is what is being used in commit I'm going to scroll between those very often, so good to have them directly after commit, and not cram in add_filter_rule in between. Then the methods those methods use are below them etc. Lowest level methods at the bottom.
For some code this rule is hard to apply, but here I think it makes lots of sense, it's a pretty clear tree of dependent functions: commit -> add_filter_rules -> add_filter_rule etc.

You can use split view, you don't need to scroll the same text editor back and forth.
I agree that some FFI helpers in between could be moved to the bottom. I reordered functions in the suggested order.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 144 at r6 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Can you change this into a real error? Just not chaining it and thus let it return the IoctlError on failure sounds reasonable. Same goes for add_redirect_rule.

Great idea! Done.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Agree. So basically we'll have to open Transaction and open_pf() to consumer.

My bad, open_pf is used internally, no need to open it.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 9 unresolved discussions.

src/transaction.rs, line 32 at r6 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
You're right, we should open PF on demand, there is no good reason to keep it alive. The only thing is, we'll have to pass File or RawFd around.

That's been done. Infallible constructor ftw!

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 55 at r6 (raw file):

        let mut pfioc_trans = unsafe { mem::zeroed::<ffi::pfvar::pfioc_trans>() };

        let all_changes = self.change_by_anchor.values().collect::<Vec<_>>();

I figured the order does not matter because once partitioned both vectors are order once and forever.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 55 at r6 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I figured the order does not matter because once partitioned both vectors are order once and forever.

are order = are ordered

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 11 unresolved discussions.

src/transaction.rs, line 39 at r7 (raw file):

    /// Commit transaction
    pub fn commit(&self) -> Result<()> {
        let fd = utils::open_pf()?;

Opening PF on demand! :tada:

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 11 unresolved discussions.

src/transaction.rs, line 85 at r6 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Actually, I can share iterator :) no need to skip

Done with help of zip() + by_ref otherwise zip consumes self

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 11 unresolved discussions.

src/transaction.rs, line 89 at r6 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Agree. Changing this.

Replaced with for-loop. - less code too. The changed code is down under now if you got confused the same as I.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 11 unresolved discussions.

src/transaction.rs, line 186 at r5 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
My bad, open_pf is used internally, no need to open it.

Removed duplicate anchor from AnchorChange.
HashMap is the primary provider of anchor names (hence used as keys).
This forced me to use a tuple (anchor_name, anchor_change) in iterations, but that's really not that bad as it looks.
Most of iterations happen over references, the upside is that we do not copy anything at all.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 10 unresolved discussions.

src/transaction.rs, line 44 at r7 (raw file):

        // partition changes by ruleset kind
        let filter_changes = self.change_by_anchor

The reason why I partition and never use self.change_by_anchor for iterations is because iterations happen in arbitrary order. It can be that two different iterations emit the same order of elements but I don't want to gamble with that. It's easier to partition and use Vec.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 11 unresolved discussions.

src/transaction.rs, line 79 at r7 (raw file):

        // create iterator for tickets
        let tickets = pfioc_elements.iter().map(|e| e.ticket).collect::<Vec<_>>();

This is unnecessary step but I wanted to limit the surface with FFI structs and rather extract tickets. It may be more optimal to iterate over pfioc_trans_e and extract ticket but I prefer code quality over insignificant performance gain here.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 12 unresolved discussions.

src/transaction.rs, line 85 at r7 (raw file):

        for (&(ref anchor_name, ref change), &ticket) in
            filter_changes.iter().zip(ticket_iterator.by_ref()) {
            for filter_rule in change.filter_rules.as_ref().unwrap().iter() {

I removed helpers that used to iterate over arrays. In the end it's less code this way.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 13 unresolved discussions.

src/transaction.rs, line 93 at r7 (raw file):

        for (&(ref anchor_name, ref change), &ticket) in
            redirect_changes.iter().zip(ticket_iterator.by_ref()) {
            for redirect_rule in change.redirect_rules.as_ref().unwrap().iter() {

This unwrap must never fail because the code above partitions two vectors by that very criteria.

Comments from Reviewable

Review status: 0 of 6 files reviewed at latest revision, 13 unresolved discussions.

src/transaction.rs, line 148 at r6 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
You can use split view, you don't need to scroll the same text editor back and forth.
I agree that some FFI helpers in between could be moved to the bottom. I reordered functions in the suggested order.

Actually removed add_redirect_rules and add_filter_rules. 3 LOC do not worth it. Used only once.

Comments from Reviewable

Reviewed 2 of 4 files at r7.
Review status: 2 of 6 files reviewed at latest revision, 11 unresolved discussions.

src/transaction.rs, line 44 at r7 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
The reason why I partition and never use self.change_by_anchor for iterations is because iterations happen in arbitrary order. It can be that two different iterations emit the same order of elements but I don't want to gamble with that. It's easier to partition and use Vec.

Yes, we should not rely on order in a HashMap since it's not guaranteed. Good decision. But the type of filter_changes is not really obvious here. The compiler can figure it out, but it's a bit complex for the reader.
How about removing ::<Vec<_>> from the collect call and instead start this declaration as
let filter_changes: Vec<(&String, &AnchorChange)> = ...

or whatever the exact type would become?
A tip for finding the type is to leave it as is, and under this assignment write:
let () = filter_changes;

Then the compiler error will tell you the exact type :) Just in case you didn't know this little trick.

src/transaction.rs, line 40 at r8 (raw file):

    pub fn commit(&self) -> Result<()> {
        let fd = utils::open_pf()?;
        let dev = fd.as_raw_fd();

It's quite confusing that the variable fd actually holds a File and the variable dev holds the fd :joy:
How about:
let pf_file = utils::open_pf()?;
let fd = pf_file.as_raw_fd();

Previously, and in other places in this crate we have used the term fd for the raw file descriptor, such as self.fd(). I think we should stick to that and not intruduce dev unless there is a good reason for that.

src/transaction.rs, line 54 at r8 (raw file):

        // create transaction elements for each ruleset and order them so elements for filter rules
        // go first followed by redirect rules

Maybe write "one transaction for each unique combination of (anchor name + RulesetKind)" to clarify what needs its own ticket. Also maybe add that this is for obtaining transaction tickets?

src/transaction.rs, line 80 at r8 (raw file):

        // fetch tickets and create iterator for them
        let tickets = pfioc_elements.iter().map(|e| e.ticket).collect::<Vec<_>>();
        let mut ticket_iterator = tickets.iter();

No need to allocate tickets and then just take the iterator unless there is a reason I'm missing. Could be just
let tickets = pfioc_elements.iter().map(|e| e.ticket);

And that would yield an iterator over just the tickets directly.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 6 unresolved discussions.

src/transaction.rs, line 85 at r7 (raw file):

        for (&(ref anchor_name, ref change), &ticket) in
            filter_changes.iter().zip(ticket_iterator.by_ref()) {
            for filter_rule in change.filter_rules.as_ref().unwrap().iter() {

It looks like you are not actually using the change for anything other than accessing change.filter_rules here and change.redirect_rules in the next for loop. So is there any reason to make filter_changes be a vector of (String, AnchorChange)? Up where you create filter_changes and redirect_changes you could use filter_map instead of filter and make them take out the rules they need instead of having a reference to the entire AnchorChange:
let filter_changes: Vec<(&str, Vec<FilterRule>)> = ...
let redirect_changes: Vec<(&str, Vec<RedirectRule>)> = ...

So they would only contain what they actually need. And then they could .take() the vectors from the AnchorChanges to not have to contain references, they can actually extract the rules completely. Then this commit method would need to take self to consume the Transaction, but that also makes sense, it does not make much sense to be able to commit transaction multiple times?
This would also get rid of the .as_ref().unwrap() part here which is not optimal in neither beauty nor safety :)

src/transaction.rs, line 93 at r7 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
This unwrap must never fail because the code above partitions two vectors by that very criteria.

See my comment on other loop for how you could get rid of this unwrap even.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 5 unresolved discussions.

src/transaction.rs, line 44 at r7 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Yes, we should not rely on order in a HashMap since it's not guaranteed. Good decision. But the type of filter_changes is not really obvious here. The compiler can figure it out, but it's a bit complex for the reader.
How about removing ::<Vec<_>> from the collect call and instead start this declaration as
let filter_changes: Vec<(&String, &AnchorChange)> = ...

or whatever the exact type would become?
A tip for finding the type is to leave it as is, and under this assignment write:
let () = filter_changes;

Then the compiler error will tell you the exact type :) Just in case you didn't know this little trick.

Thanks for the hint.  Done

src/transaction.rs, line 85 at r7 (raw file):
Previously, faern (Linus Färnstrand) wrote…
It looks like you are not actually using the change for anything other than accessing change.filter_rules here and change.redirect_rules in the next for loop. So is there any reason to make filter_changes be a vector of (String, AnchorChange)? Up where you create filter_changes and redirect_changes you could use filter_map instead of filter and make them take out the rules they need instead of having a reference to the entire AnchorChange:
let filter_changes: Vec<(&str, Vec<FilterRule>)> = ...
let redirect_changes: Vec<(&str, Vec<RedirectRule>)> = ...

So they would only contain what they actually need. And then they could .take() the vectors from the AnchorChanges to not have to contain references, they can actually extract the rules completely. Then this commit method would need to take self to consume the Transaction, but that also makes sense, it does not make much sense to be able to commit transaction multiple times?
This would also get rid of the .as_ref().unwrap() part here which is not optimal in neither beauty nor safety :)

Since AnchorsChanges are stored in HashMap, it's not that easy to modify them, .take() modifies the field.
There were multiple mutable borrows happening at the same time so I gave up on untangling this.
The alternative was to simply switch to filter_map and produce pairs of (&String, &Vec<XXXRule>). At this point all refs and unwraps are gone and we don't modify anything.
commit is self-consuming now which makes sense in a way. One could argue that transaction may be repeated if it failed but we may as well create the entire transaction once again.

src/transaction.rs, line 40 at r8 (raw file):
Previously, faern (Linus Färnstrand) wrote…
It's quite confusing that the variable fd actually holds a File and the variable dev holds the fd :joy:
How about:
let pf_file = utils::open_pf()?;
let fd = pf_file.as_raw_fd();

Previously, and in other places in this crate we have used the term fd for the raw file descriptor, such as self.fd(). I think we should stick to that and not intruduce dev unless there is a good reason for that.

Done.

src/transaction.rs, line 54 at r8 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Maybe write "one transaction for each unique combination of (anchor name + RulesetKind)" to clarify what needs its own ticket. Also maybe add that this is for obtaining transaction tickets?

Done.

src/transaction.rs, line 80 at r8 (raw file):
Previously, faern (Linus Färnstrand) wrote…
No need to allocate tickets and then just take the iterator unless there is a reason I'm missing. Could be just
let tickets = pfioc_elements.iter().map(|e| e.ticket);

And that would yield an iterator over just the tickets directly.

Done.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 5 unresolved discussions.

src/transaction.rs, line 40 at r8 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Done.

Normally File is a wrapper around file descriptor in its essence, there is also RawFd which is OS-speciifc type of filesystem descriptor.
Since /dev/pf is a pseudo-file for virtual device it felt natural to call it device.
The fact that file system is ~ab~used instead of having a reasonable socket solution makes me wonder what the heck... but let's keep it the way it used to be, pf_file and fd for raw descriptor.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 5 unresolved discussions.

src/transaction.rs, line 40 at r8 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Normally File is a wrapper around file descriptor in its essence, there is also RawFd which is OS-speciifc type of filesystem descriptor.
Since /dev/pf is a pseudo-file for virtual device it felt natural to call it device.
The fact that file system is ~ab~used instead of having a reasonable socket solution makes me wonder what the heck... but let's keep it the way it used to be, pf_file and fd for raw descriptor.

:laughing:

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 85 at r7 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Since AnchorsChanges are stored in HashMap, it's not that easy to modify them, .take() modifies the field.
There were multiple mutable borrows happening at the same time so I gave up on untangling this.
The alternative was to simply switch to filter_map and produce pairs of (&String, &Vec<XXXRule>). At this point all refs and unwraps are gone and we don't modify anything.
commit is self-consuming now which makes sense in a way. One could argue that transaction may be repeated if it failed but we may as well create the entire transaction once again.

If you actually take out the values, by ownership in each iter_mut, like I propose in a post further up. Then you won't have multiple mutable borrows, because one borrow will end before the next one start. That would require cloning of the anchor names, but performance is not super important here. I don't know which one would look best, but awesome you got rid of the unwraps at least!

src/transaction.rs, line 48 at r9 (raw file):

            .filter_map(
                |(key, ref change)| if let Some(ref filter_rules) = change.filter_rules {
                    Some((key, filter_rules))

Since change.filter_rules is already an option you here destructure an option and then re-create one. I think it should be possible to do something like:
let filter_changes = Vec<(String, Vec<FilterRule>)> = self.change_by_anchor
    .iter_mut()
    .filter_map(|(anchor, mut ref change)| {
        change.filter_rules.take().map(|rules| (anchor.clone(), rules))
    });

To actually take the vector out, the ownership of it, so you can deal less with references. I don't know exactly if that will make life easier further down in the function, I'm just trying to figure out if there are ways to make the types here more straight forward. Since we consume the data anyway there is not very much reason to just take refs.
For immutable data references like what you have in the code now, the convention is to take the slice &[FilterRule] instead of &Vec<FilterRule>. You can easily do this by adding & and [..] before and after filter_rules.

src/transaction.rs, line 186 at r9 (raw file):

pub struct AnchorChange {
    pub filter_rules: Option<Vec<FilterRule>>,
    pub redirect_rules: Option<Vec<RedirectRule>>,

The fields does not have to be public when you have setters for them in the impl block. Makes sense to hide them, and not talk about Some vs None in the documentation. The "encoding" that None means no change and Some(vec![]) mean flush is not very user friendly and be better hidden as an implementation detail rather than shown in the public API in my opinion.
Would be nice to add documentation to the two setters that if one call that and set them then that kind will get flushed. Would be even better if there were an explicit AnchorChange::flush_filter_rules() and similar for redirect.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 48 at r9 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Since change.filter_rules is already an option you here destructure an option and then re-create one. I think it should be possible to do something like:
let filter_changes = Vec<(String, Vec<FilterRule>)> = self.change_by_anchor
    .iter_mut()
    .filter_map(|(anchor, mut ref change)| {
        change.filter_rules.take().map(|rules| (anchor.clone(), rules))
    });

To actually take the vector out, the ownership of it, so you can deal less with references. I don't know exactly if that will make life easier further down in the function, I'm just trying to figure out if there are ways to make the types here more straight forward. Since we consume the data anyway there is not very much reason to just take refs.
For immutable data references like what you have in the code now, the convention is to take the slice &[FilterRule] instead of &Vec<FilterRule>. You can easily do this by adding & and [..] before and after filter_rules.

I forgot some punctuation and a few words. It should have been:
Since change.filter_rules is already an option you can simplify this. Here you destructure an option and then re-create one.
Also there should have been an extra .collect() at the end of course.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 48 at r9 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I forgot some punctuation and a few words. It should have been:
Since change.filter_rules is already an option you can simplify this. Here you destructure an option and then re-create one.
Also there should have been an extra .collect() at the end of course.

About &Vec<T> and conventions. You also have &String, where the more idiomatic type would be &str if you want to borrow it. Otherwise you can get less references and more straight forward types if you just clone the strings and go with String all the way.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 186 at r9 (raw file):
Previously, faern (Linus Färnstrand) wrote…
The fields does not have to be public when you have setters for them in the impl block. Makes sense to hide them, and not talk about Some vs None in the documentation. The "encoding" that None means no change and Some(vec![]) mean flush is not very user friendly and be better hidden as an implementation detail rather than shown in the public API in my opinion.
Would be nice to add documentation to the two setters that if one call that and set them then that kind will get flushed. Would be even better if there were an explicit AnchorChange::flush_filter_rules() and similar for redirect.

Good point, made those fields private.
Transactions replace rulesets in anchors, so flush is not a special case, but rather equivalent to passing an empty array instead of array of new rules. I don't see ambiguity here. It seemed rather odd when we started working with transactions because we had low-level implementation details exposed, but now when all of that buried down under Transaction logic this is quite straightforward. That's why arrays of rules are Optional

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 85 at r7 (raw file):
Previously, faern (Linus Färnstrand) wrote…
If you actually take out the values, by ownership in each iter_mut, like I propose in a post further up. Then you won't have multiple mutable borrows, because one borrow will end before the next one start. That would require cloning of the anchor names, but performance is not super important here. I don't know which one would look best, but awesome you got rid of the unwraps at least!

Done.

src/transaction.rs, line 48 at r9 (raw file):
Previously, faern (Linus Färnstrand) wrote…
About &Vec<T> and conventions. You also have &String, where the more idiomatic type would be &str if you want to borrow it. Otherwise you can get less references and more straight forward types if you just clone the strings and go with String all the way.

OK that's great to know! Thanks!
I still keep Vec around since when we take() we can't simply reference &rules[..] anymore since it won't live long enough.

Comments from Reviewable

Review status: 2 of 6 files reviewed at latest revision, 2 unresolved discussions.

src/transaction.rs, line 48 at r9 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
OK that's great to know! Thanks!
I still keep Vec around since when we take() we can't simply reference &rules[..] anymore since it won't live long enough.

Yes. I meant you should keep the Vec in filter_changes. I just showed that if you wanted reference to array data it would have been better to go with &[T] than &Vec<T> since it's more idiomatic.
This looks good now!

src/transaction.rs, line 175 at r10 (raw file):

/// Structure that describes anchor rules manipulation allowing for targeted changes in anchors.
/// Rules that are set to Some() will replace corresponding ruleset in anchor, while
/// rules that set to None will remain untouched during transaction.

Please update documentation as I suggested when I wrote to make the fields private. Now this struct does not expose any Option anywhere, so it's strange to talk about Some and None, when the user of this struct can actually never see them.
Instead of None it can be written that not setting anything for that rule type will leave the current rules in place. instead of Some it can be written that setting some rules for that type will flush current rules and replace them with the new ones. I think it should be pointed out that setting an empty vec will be equal to removing the rules of that type. It's a bit hard to express the full intent without ambiguity I think. Since writing what I wrote above: "Not setting anything for that rule type will leave the current rules in place" can be interpreted as setting it to nothing.

Comments from Reviewable

Reviewed 1 of 4 files at r3, 1 of 1 files at r4, 1 of 4 files at r7, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

tests/transaction.rs, line 61 at r10 (raw file):

        .action(pfctl::RedirectRuleAction::Redirect)
        .to(pfctl::Endpoint::from(pfctl::Port::from(3000)))
        .redirect_to(pfctl::Endpoint::from(pfctl::Port::from(4000)))

Would it not be enough with just .to(Port::from(3000)) and equivalent. Endpoint implements From<Port> so the rest should be automatic.

tests/transaction.rs, line 94 at r10 (raw file):

        vec![
            "block drop inet from any to <IP_ADDRESS>",
            "block drop inet from any to <IP_ADDRESS>",

Feels like "dangerous" test rules. They will block very much and if there is any problem with the tests we might destroy network access for the computer running the tests. Would it hurt the tests themselves if the rules are designed to be less likely to actually interfere with the actual network no the computer? Such as only using pass rules instead of block or making the matching more specific, such as from <IP_ADDRESS> port 99 to <IP_ADDRESS> port 999 or something.

tests/transaction.rs, line 166 at r10 (raw file):

    assert_eq_filter_rules(pfcli::get_rules(ANCHOR2_NAME).unwrap());
    assert_eq_redirect_marker_rule(pfcli::get_nat_rules(ANCHOR2_NAME).unwrap());

I find this test very hard to read actually :( I don't have a concrete recommendation for how to improve it. But the entire test is more or less just assert_*. Even adding the rules are asserts even though they are not the actions being tested.

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions.

src/transaction.rs, line 48 at r9 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Yes. I meant you should keep the Vec in filter_changes. I just showed that if you wanted reference to array data it would have been better to go with &[T] than &Vec<T> since it's more idiomatic.
This looks good now!

I see. I agree with you, thanks!

tests/transaction.rs, line 166 at r10 (raw file):
Previously, faern (Linus Färnstrand) wrote…
I find this test very hard to read actually :( I don't have a concrete recommendation for how to improve it. But the entire test is more or less just assert_*. Even adding the rules are asserts even though they are not the actions being tested.

What if when we add rules we simply .unwrap() instead of matching against Ok(()). This should improve readability in the first place. Not sure how helpful that would be in case of errors, but given we test adding rules in separate unit test then it should be probably fine.

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

src/transaction.rs, line 175 at r10 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Please update documentation as I suggested when I wrote to make the fields private. Now this struct does not expose any Option anywhere, so it's strange to talk about Some and None, when the user of this struct can actually never see them.
Instead of None it can be written that not setting anything for that rule type will leave the current rules in place. instead of Some it can be written that setting some rules for that type will flush current rules and replace them with the new ones. I think it should be pointed out that setting an empty vec will be equal to removing the rules of that type. It's a bit hard to express the full intent without ambiguity I think. Since writing what I wrote above: "Not setting anything for that rule type will leave the current rules in place" can be interpreted as setting it to nothing.

Ah I see what you mean, OK I'll update documentation.

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

tests/transaction.rs, line 61 at r10 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Would it not be enough with just .to(Port::from(3000)) and equivalent. Endpoint implements From<Port> so the rest should be automatic.

Right, I think I had IPs before but then I removed them, will refine that..

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

tests/transaction.rs, line 94 at r10 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Feels like "dangerous" test rules. They will block very much and if there is any problem with the tests we might destroy network access for the computer running the tests. Would it hurt the tests themselves if the rules are designed to be less likely to actually interfere with the actual network no the computer? Such as only using pass rules instead of block or making the matching more specific, such as from <IP_ADDRESS> port 99 to <IP_ADDRESS> port 999 or something.

I agree, we should have rules that are less likely to break the networking.

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions.

tests/transaction.rs, line 166 at r10 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
What if when we add rules we simply .unwrap() instead of matching against Ok(()). This should improve readability in the first place. Not sure how helpful that would be in case of errors, but given we test adding rules in separate unit test then it should be probably fine.

Yes, that would likely improve readability. I know I argued for using assert_matches earlier to get better error messages, but apparently that did not scale for tests this large.

Comments from Reviewable

Review status: 5 of 6 files reviewed at latest revision, 3 unresolved discussions.

tests/transaction.rs, line 61 at r10 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
Right, I think I had IPs before but then I removed them, will refine that..

Updated rules to use from IP to decrease an impact on computer

tests/transaction.rs, line 94 at r10 (raw file):
Previously, pronebird (Andrei Mihailov) wrote…
I agree, we should have rules that are less likely to break the networking.

Done.

tests/transaction.rs, line 166 at r10 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Yes, that would likely improve readability. I know I argued for using assert_matches earlier to get better error messages, but apparently that did not scale for tests this large.

Either way, I moved some of assert_matches! into helper functions + get_nat_rules() and such that is always the same, the only variable there is anchor.

Comments from Reviewable

Reviewed 1 of 2 files at r11.
Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Reviewed 1 of 2 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable


pathlib format support for read/write

Now it supports only strings as path argument, Path format support will be a great feature!

Thank you for your suggestion! Pathlib support was added in PR https://github.com/JIy3AHKO/pyfastmask/pull/8 and is available in version 1.2.0


Text fields in search function in security display white letters on white background, hence not visible

Duplicate of https://github.com/silverstripe/silverstripe-cms/issues/1482


Paper page should list the authors

Scenario
Given a paper with an author
Then no error should occour
Error

Got NoMethodError: undefined method `authors=' for #Paper:0x00000007e179e8
Did you mean?  authors
Did you mean?  authors

Estimated progress: 91% complete

Sorry, it's not working for me yet:
Scenario
Given a paper with an author
When a user visits the paper page
Then it should show the author's name
Error

Expected to find text "alan turing" in "title: computing machinery and intelligence venue: mind 49: 433-460 year: 1950"

If you have problems solving this task, please don't hesitate to contact the teaching team!

Sorry, it's not working for me yet:
Scenario
Given a paper with an author
When a user visits the paper page
Then it should show the author's name
Error

Expected to find text "alan turing" in "title: computing machinery and intelligence venue: mind 49: 433-460 year: 1950"

If you have problems solving this task, please don't hesitate to contact the teaching team!


P2. Add an edit game button to expanded game page

There is currently no way to edit, clone, or delete the game when the game window is expanded. I think the best course of action is to add a slightly larger 3-dot icon next to the game's title that will have the same drop down as the smaller ones, containing the edit, clone, and delete functions. Pictured below is the location for the icon.

Example of the the dots position. They may have to be smaller than what is shown here, but slightly larger than the three dot menus on each of the question tabs.


Add common hex encode/decode fuzz tests

Signed-off-by: Lin Lin<EMAIL_ADDRESS>For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

/retest


New feature: attach stack trace?

For debug purposes, it would be nice to add a mode to Mutiny which attaches stack traces to exceptions, to record where we created the Uni, rather than when it failed, which often doesn't have the right stack trace.
In the past, I've used this extensively:
    private static <V> CompletionStage<V> attachStackTrace(CompletionStage<V> cs) {
        Throwable exception = new RxException("Rx operation failed");
        return cs.exceptionally(x -> {
            exception.initCause(x);
            rethrow(exception);
            return null;
        });
    }

But we should make this automatic, perhaps with a global setting/config?

I really want this feature. However, it would require the Stack Walker API (JAva 9). Still trying to find a good way to handle both Java 8 and Java 11.

Why, the strategy I just showed works fine.

StackWalker is good when you to filter out elements like reflection, and when you want to be sure to get the complete stack trace because the VM can in theory drop stack elements.
Otherwise the pre-Java 9 technique that @FroMage suggests is all we need.

I've done a first attempt and the result was not great. I will try to see how it can be improved.

I've done some experiments and it's a can of worms 😏
Here are some notes and brain dump.
It's not too hard to rewrite stack traces, but you can't simply presume that the top of the stack minus 1 is where you where actually called in the source code. Synthetic methods can mix in, and the group / operator nature of the Mutiny API does complicate things, not to mention that some operators are sugar on top of other operators. Still, with the name of the operator you are in you can safely descend from the top of the stack and you should be on the good stack frame.
Another problem is how to keep track of the captured stack frames, and make sure that you don't excessively capture stack frames (again, sugar operators make it more complex). This requires some invasive changes in the internals.
Next thing is how to propagate exceptions. Should you propagate the real exception and attach the stacktrace capture? Or should you propagate a wrapper exception? And in the later case what happens if you are in a sugar operator? (hint: you could propagate a rewriting stack trace which is not that from the user code but that from the sugar operator implementation.
Last but not least depending on what happens when an exception is being propagated we may need to double the tests suite to cover the cases where we have no rewritings and the cases where we do rewrite, possibly forwarding a wrapping exception.
Perhaps it's just me who's had inconclusive ideas for the moment, so I'll let it aside for a while and come back to it later to see if I can come up with another angle at that problem.
Nevertheless I'd like to make sure we don't render the code base unnecessarily complicated. It'd be nice to have that feature, but not at any cost.
And let's not forget that given this:
Uni.createFrom().item(123)
        .call(() -> {
            throw new RuntimeException("woops");
        })
        .subscribe().with(
                value -> System.out.println(value),
                err -> err.printStackTrace());

...well the stack trace points to the actual place where the failure happened:
java.lang.RuntimeException: woops
	at io.smallrye.mutiny.helpers.stacktrace.UniStackFrameRewritingTest.lambda$basic$0(UniStackFrameRewritingTest.java:14)
	at io.smallrye.mutiny.groups.UniOnItem.lambda$call$3(UniOnItem.java:120)
	at io.smallrye.mutiny.groups.UniOnItem.lambda$call$2(UniOnItem.java:99)
	at io.smallrye.mutiny.operators.uni.UniOnItemTransformToUni$UniOnItemTransformToUniProcessor.performInnerSubscription(UniOnItemTransformToUni.java:68)
	at io.smallrye.mutiny.operators.uni.UniOnItemTransformToUni$UniOnItemTransformToUniProcessor.onItem(UniOnItemTransformToUni.java:57)
	at io.smallrye.mutiny.operators.uni.builders.UniCreateFromKnownItem$KnownItemSubscription.forward(UniCreateFromKnownItem.java:38)
	at io.smallrye.mutiny.operators.uni.builders.UniCreateFromKnownItem$KnownItemSubscription.access$100(UniCreateFromKnownItem.java:26)
	at io.smallrye.mutiny.operators.uni.builders.UniCreateFromKnownItem.subscribe(UniCreateFromKnownItem.java:23)

(...)

It's just 1 example and you can easily come up with a problematic stack trace example, but not everything is bad with the current implementation.

I removed this from 0.15.0.


Add a primitive Linalg TileAndDistributeOnTensors pass.

The TileAndDistributeOnTensors pass performs tiling and distribution to a 3-D grid of processors identified by their WorkgroupIdOp in a range of [0, WorkgroupSizeOp).
This results in a parametric tiling that currently does not allow further canonicalizations.
PiperOrigin-RevId: 345629018

@pifon2a


web.py doesn't initialize with AOO 4.1.2 on Win7

OpenOffice 4.1.2 (i.e. python 2.7.6) on Windows 7.
MRI does not launch and complains with the following traceback:
_"C:\Users\127998\AppData\Roaming\OpenOffice\4\user\uno_packages\cache\uno_packages
\sv59ol0.tmp_\MRI-1.3.3.oxt\pythonpath\mytools_Mri\web.py", line 104, in __init__
    super().__init__(cast, config)
TypeError: super() takes at least 1 argument (0 given)_

Modifying the line 104 with something like super(DoxygenIDLRef, self).__init__(cast, config)  works fine.
Thanks.

Wonder why you choose doxygen check box for AOO 4.1.2 reference.
But fixed on rev 598c22c3406222eb605394e63eaba65e418e7599.
Thank you for the report.

You're right, the doxygen option was checked by error, but as MRI didn't launch anymore, I wasn't able to fix it in the gui...
Thank for taking care of it !


Yarn project building fails with NPE on yarnInstallDirectoryPath

Hi,
Description
When building a Yarn project, installFrontend fails with NPE (stacktrace snippet below).
Tracing the problem,  in FrontendGradlePlugin, yarnInstallDirectoryPath is not initialized because its initializations are conditioned on task.getYarnEnabled().get(), which is apparently never true at the time of these initializations.
Removing these conditions from around all the initializations of yarnInstallDirectoryPath fixes the problem and the build succeeds.
Stacktrace:
Caused by: java.lang.NullPointerException
at org.siouan.frontendgradleplugin.domain.usecase.ResolveExecutionSettings.execute(ResolveExecutionSettings.java:100)
at org.siouan.frontendgradleplugin.infrastructure.gradle.adapter.GradleScriptRunnerAdapter.execute(GradleScriptRunnerAdapter.java:35)
at org.siouan.frontendgradleplugin.infrastructure.gradle.AbstractRunCommandTask.execute(AbstractRunCommandTask.java:92)
at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:104)
at org.gradle.api.internal.project.taskfactory.StandardTaskAction.doExecute(StandardTaskAction.java:49)
Environment

OS: Windows 10
JDK: 8 64 bits
Gradle: Gradle 6.3, Gradle 6.4
Frontend Gradle plugin: latest pulled from GitHub, 2.0.0, 2.1.0

Hi @nitzan-n,
Thank you for your message.
Actually, I don't see why the yarnInstallDirectoryPath variable would be null at this time: the installFrontend task is being executed, which means it has been configured by Gradle, i.e. the yarnEnabled property, if true, leads to the initialization of this variable in the FrontendGradlePlugin class.
I wasn't able to reproduce the issue with Gradle 5.1 (minimum supported version) and Gradle 6.4.

Can you provide the exact configuration of the plugin in your test case?
Does the issue occurs every time, or occasionally?
If you tried with release 2.1.0, can you add a functional test in the InstallTaskFuncTest class that reproduces the issue?
If not possible, can you provide an archive of the project where the issue occurs regularly?

Thanks in advance for your help.
BR,
Vincent

I forgot one thing: can you provide the Gradle's output when executing the task with debug level (-d flag)?

Hi,
Thanks for your work on the project and your quick response.
I managed to re-create the issue with the attached gradle.build file.
Specifically, there seems to be some interaction with another plugin in the project, 'nebula.clojure', and the 'org.siouan.frontend' plugin:

When 'nebula.clojure' is activated (setting line 16 to: apply plugin: 'nebula.clojure') the conditional initialization of the Yarn install path doesn't happen, and all the Frontent build task fail with NPE (see attached file).
When 'nebula.clojure' is not activated (setting line 16 to: apply plugin: 'java') the conditional initialization of the Yarn install path happens, and the build tasks succeed.

At this point I don't know which plugin is 'to blame' for this, or if there are other plugins that may cause this interaction with 'org.siouan.frontend' when they are also applied with it in the same project.
However, as I suggested, removing the conditions from around all the initializations of Yarn install directory in class FrontendGradlePlugin
(the lines: task.getYarnInstallDirectory().set(extension.getYarnInstallDirectory()))
solves this issue.
As far as I can tell, there is no drawback to doing so. So at the very least, it should improve "isolation"(?) between the 'frontend' plugin and other project plugins with no cost.
Thanks.
build.txt
build.gradle.txt

Hi,
Thank you for this great analysis! I will continue the job to understand why both plugins cannot live together, using the build definition you provided.
I also agree with the fix you suggested, and I already started to remove the conditional initialization of the yarnInstallDirectory variable. Initially, this condition was necessary due to how Gradle was validating task I/O. But I found a way to handle things differently, and it seems to be simpler now.
I will provide the fix in release 3.0 at the beginning of the week.
Thanks again for your kind help!
BR,
Vincent

For information: the related discussion in Gradle forums.


error when compile with " USE_DIST_KVSTORE = 1 "

when I compile MXNET with the USE_DIST_KVSTORE set to 1 in config.mk file, I got the following error message:
include/mxnet/kvstore.h:172:32: error: ‘ps::Environment’ has not been declared
...
Anyone knows why?
thanks

Same problem here

This might because the ps-lite in the gitsubmodule is not update to date with https://github.com/dmlc/ps-lite. The gitsubmodule has is on (7faaeb73bcb9d68b464186d3191494aa95243703) while the head is on (8aff164580f0e4ff81ad98038b6ec4ec02452ce8)
Just update ps-lite by pulling from the master branch can solve this problem

thanks @ranxian I will try.


Database Driven Confguration

Allow for configuration of rollbar, twilio, and importer to come from the database.

Resolved by 0.3.0-beta1


AppArmor: Add read permission for executables

What type of PR is this?
/kind bug
What this PR does / why we need it:
Depending on how processes are spawned, ix is not enough and we also need to explicitly permit reading the executable.
Which issue(s) this PR fixes:
None
Does this PR have test?
N/A
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Fix a bug where recorded AppArmor profiles would prevent executables from spawning.

Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 41.29%. Comparing base (11d77f4) to head (7b552de).
Report is 475 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2554      +/-   ##
==========================================
- Coverage   45.50%   41.29%   -4.22%
==========================================
  Files          79      109      +30
  Lines        7782    18234   +10452
==========================================
+ Hits         3541     7529    +3988
- Misses       4099    10208    +6109
- Partials      142      497     +355

/test pull-security-profiles-operator-test-e2e

/ok-to-test

@mhils If this doesn't pass could you please rebase it on main branch, we removed the image scanner for now because it seems that trivy rate limited their database image and is now causing a lot of timeout errors.
https://github.com/aquasecurity/trivy-action/issues/389

Please rebase to fix the CI.

/lgtm


Pass dataset and index to render method

In 086b7f6 you pass the dataset and index to the fontColor method. These would also be useful for the render method, as there can be multiple datasets in one chart, and currently there is no way to know which one you are rendering now.

ok, will add in next version

add this in v0.8.2


Update readme

Description
Update the links to the examples in the Readme, after restructuring the examples folder.
Issue reference
Fixes #282
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:

Updated the CHANGELOG.md in reverse chronological order (newest at the top) with a concise description of the changes, including the PR number.
Noted any breaking changes, including details on how it might impact existing functionality.

Type of change

[ ] New Feature: A non-breaking change that adds new functionality.
[ ] Optimization: A code change that improves performance.
[ ] Examples: A change to existing or additional examples.
[ ] Bug Fix: A non-breaking change that addresses an issue.
[ ] Documentation: Updates to documentation or new documentation for new features.
[ ] Refactoring: Non-functional changes that improve the codebase.
[ ] Style: Non-functional changes related to code style (formatting, naming, etc).
[ ] Testing: Additional tests to improve coverage or confirm functionality.
[ ] Other: (Insert description of change)

Key checklist:

[ ] No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
[ ] All unit tests pass: $ nox -s tests
[ ] The documentation builds: $ nox -s doctest

You can run integration tests, unit tests, and doctests together at once, using $ nox -s quick.
Further checks:

[ ] Code is well-commented, especially in complex or unclear areas.
[ ] Added tests that prove my fix is effective or that my feature works.
[ ] Checked that coverage remains or improves, and added tests if necessary to maintain or increase coverage.

Thank you for contributing to our project! Your efforts help us to deliver great software.

The integration tests show an intermittent failure, but since this pull request only updates links in the Readme, I will merge it.


[Enhancement] Additional command properties (treated like version)

There's often other things like major dependency versions and other critical application properties that you'd want listed next to the app version, so I'd be nice to have a simple workflow for adding that.

Hi @josh-hemphill,
do you want to show the extra properties in the help text or only with the version option.
Actually you could do something like this, if you want to show the extra properties only with the long version option:
await new Command()
  .version("0.1.0")
  .versionOption("-V, --version", "Print version.", {
    action() {
      const isLong = this.getRawArgs().includes(`--version`);
      if (isLong) {
        console.log(
          `${this.getName()} ${this.getVersion()}\n` +
          `deno ${Deno.version.deno}\n` +
          `v8 ${Deno.version.v8}\n` +
          `typescript ${Deno.version.typescript}`
        );
      } else {
        console.log(this.getVersion());
      }
    }
  })
  .parse(Deno.args);

Maybe it makes sense to add a .longVersion() method to make it simpler to to somthing like this.
If you have any preferred way of doing this feel free to make a suggestion 😉

Hi,
Having that additional info in the Help text is the part that would require additional workflow or method changes.
Yeah, that's the route I went for providing that info elsewhere, but I'd like to have some of the most critical info in the help text, formatted similarly to the version, and the only way I could think to that would be to try and replace the help text and split the default help text on the version and insert the additional stuff, but that's going add some more bulky code to my entry cli file, so I was hoping that this kind use case could warrant adding something to the lib to make it cleaner and maybe similar to the version, or just extend the version method with an optional object of key/value pairs to add additional critical application properties and format them similarly to the version.

hey @josh-hemphill,
i have added a new .meta() method which can be used to add additional information to the help output. You can test it in the command/meta branch: https://github.com/c4spar/deno-cliffy/blob/command/meta/command/README.md#additional-info. If you have any suggestions just let me know 😉


[Good First Issues]: implement function "check_custom_values" - 7 points

Motivation
Vanus Functions are a series of pre-built functions worked within Vanus transformer. By choosing a function, users can specify the way that events will be changed.
It will be a good start if you decide to contribute to Vanus codes.
Details
Function name: check_custom_values
Args

sourceJsonPath - The name of an existing column.
customValue - Custom value to check
targetJsonPath The name of the new column to be created.
trueFlagReplacement - custom string if value equal custom value
falseFlagReplacement - Custom string if value doesn't equal custom value

Description
The function is used to check if the source JSON path contains the customValue or not. If it contains that value, the trueFlagReplacement will be assigned to the target JSON path. If it doesn't, the falseFlagReplacement will be assigned to the target JSON path.
Example
{
    "specversion" : "1.0",
    "type" : "com.example.someevent",
    "source" : "/mycontext",
    "subject": null,
    "id" : "C234-1234-1234",
    "time" : "2018-04-05T17:31:00Z",
    "comexampleextension1" : "value",
    "comexampleothervalue" : 5,
    "datacontenttype" : "application/json",
    "data" : {
        "appinfoA" : "",

    }
}

After the function "check_custom_values":
"pipeline":[
    {"command":["extract_custom_values","$.data.appinfoA", "$.data.appinfoC", "The element has content.", "The element is empty"]}
]

The transformed event should look like this:
{
    "specversion" : "1.0",
    "type" : "com.example.someevent",
    "source" : "/mycontext",
    "subject": null,
    "id" : "C234-1234-1234",
    "time" : "2018-04-05T17:31:00Z",
    "comexampleextension1" : "value",
    "comexampleothervalue" : 5,
    "datacontenttype" : "application/json",
    "data" : {
        "appinfoA" : "",
        "appinfoC" : "The element is empty"

How to implement

Fork the vanus repository.
Create a branch function-check_custom_values
Create check_custom_values.go under internal/primitive/transform/action/strings, and then implement the function.
Create check_custom_values .go under internal/primitive/transform/action/strings for unit test.
Register your function in internal/primitive/transform/runtime/init.go.
Submit your PR

Contribution points
Each issue counts specific points based on its difficulty. Contributors earn the corresponding points if they solve that issue. Contribution points are used to describe the contributions contributors have made.  They can also be used to get rewards when there are community events.
This issue counts 7 points.
How to claim to solve the issue
If you want to implement this function, please leave a comment in this issue like:

I'd like to implement this function, please assign this issue to me.

Vanus community will assign the issue to you on time.

would like to do this, please assign this issue to me thank you

Hi @dhanusaputra, Thanks for your interest in developing a function. Join our slack community for better communication.

sure will join the slack, thank you


Streaming audio

The streaming audio record doesn't seem to record something in my case.
Do you know why ?

It seems the SDK did not connect to OpenAI properly, to "whisper" those data.


Diacritics don't work properly

If using the Hungarian diacritic layout, it only writes "á" "é" "ó" and "ú" in upper case.
Note that it doesn't affect "ö" "ő" "ü" and "ű".
There is also no way of typing "í".
I'm using 8VIM iteration-11.
OS: MIUI <IP_ADDRESS>.TKOEUXM

In the Portuguese layout, all the "a"s are made in the opposite direction. While we should do the movement CENTER-RIGHT-TOP-RIGHT-CENTER to get "á", we actually need to do the movements as if we were trying to get "y" with diacritics. So it's working, but with the wrong movement, like CENTER-TOP-RIGHT-TOP-CENTER to get "á". And the same is happening if we want all the other "a" diacritics.

In the Portuguese layout, all the "a"s are made in the opposite direction. While we should do the movement CENTER-RIGHT-TOP-RIGHT-CENTER to get "á", we actually need to do the movements as if we were trying to get "y" with diacritics. So it's working, but with the wrong movement, like CENTER-TOP-RIGHT-TOP-CENTER to get "á". And the same is happening if we want all the other "a" diacritics.

Just checked it.
Maybe it works in Portuguese but it doesn't work in Hungarian.
Also if it works like that in Portuguese is still an issue beacause CENTER-RIGHT-TOP-RIGHT-CENTER should be the right movement. CENTER-TOP-RIGHT-TOP-CENTER is not intuitive for "á" at all.

In the Portuguese layout, all the "a"s are made in the opposite direction. While we should do the movement CENTER-RIGHT-TOP-RIGHT-CENTER to get "á", we actually need to do the movements as if we were trying to get "y" with diacritics. So it's working, but with the wrong movement, like CENTER-TOP-RIGHT-TOP-CENTER to get "á". And the same is happening if we want all the other "a" diacritics.

In my case, the issue was solved with the last Iteration on PlayStore, which changed they steps to access the diacritics layouts.


Position of name and value makes it difficult to quickly read metatag values.

I'm willing to submit a pull request for this.
Right now the extended  tags (src/components/ExtendedTags.astro) output from this project output with the content before the name and that makes it difficult to parse at a glance.
      content={content}
      http-equiv={httpEquiv}
      media={media}
      name={name}
      property={property}
    />

I'd think it would make it easier if the name and property were the first attributes filled there.

@mikeethedude Thank you for your input! Since this input seems to stem from your experience with using this feature in your project(s), I would be more than happy to merge a pull request improving ergonomics this way. Please feel free to go ahead with this, and I'll put it into the next version!

Awesome! Pull request is open. I think this is a pretty simple swap on properies. See https://github.com/jonasmerlin/astro-seo/pull/85

Landed in 0.8.1


[github-actions] remove coverage artifacts after uploading

This removes artifacts when they are no longer needed, saves storage space.

Because GitHub API can't tell if the request and the target artifact are from the same PR, it doesn't allow workflows to use GITHUB_TOKEN to delete artifacts. So it seems infeasible to do this. The only approach I can find is https://github.com/marketplace/actions/delete-run-artifacts which involves taking a personal access token, and I'm not sure how secure it is.

Found this Delete Artifact. It seems perfectly suited for this purpose. I'm trying it out.

Looks like it's working well. Marking this PR as ready for review.


Feature/ds 392

OverviewTile are now independent for better flexibility.
OverviewTile are now used in the composed component section of the doc for better readability.
A composedComponent component has been developed to easily document them within mdx files.
Icon documentation has been added.
We are now resolving documentation present in the packages/icons folder.

Something odd is happening here.
For this section, i would
1- remove Tag and TagList from the composed components. There defined in this file already, no need to point to it
2- Merge them all together

Can you pass all articles annd make sure that this pattern doesn't exist anymore:

1- there is still bullet points for some reasons
2- under Composed Components, i only want a list of component, not another sub section


Extend randomString helper

Extends randomString helper:

[x] pass meaningful object instead of bunch of booleans
[x] add option to generate strings with whitespaces (full text imitation).

overall :+1:


[WIP] Add support for node readiness taints

Implement #628 adding support for node readiness taints.
Issue, if available:

#628

Description of changes:
This PR adds support for node readiness taints as described in #628.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Closing as stale. Happy to reopen this discussion in the future.

any more thoughts/motivation on this approach?  i'm stuck on the original problem while trying to implement Cilium in my cluster via taints.

Which version of Cilium? I was using Cilium and Karpenter in my cluster up until a few weeks ago.

1.11.3 -- without using the node taint method, I end up with pods which are do not have matching CiliumEndpoints (which basically means I cannot ensure any of the NetworkPolicy goodness is applied to these nodes).

Makes sense, thanks for clarifying!


Add a list of protected PVCs to DRPC status

Please look only at the last commit for review (based on top of #427 )

(rebased in order and merged post @BenamarMk review)


add eq10q: LV2 EQ plugins and more, with 64 bit processing

Look good to me.
cc @cillianderoiste

What about travis?

Built locally, thanks.


remove manual bridge config

So, the whole problem with the bridge stuff was just the order of the environment files. It was picking up FLANNEL_SUBNET from k8s.conf instead of from subnet.env
Tested on a 7 node cluster, confirmed fix.

Of course, how could I miss that?
Thanks for spotting it.
LGTM


[Bug] Move Prover proves wrong spec for shifting operator

🐛 Bug
Move prover proves  wrong spec for shifting operator.
To reproduce
Move prover can successfully find the verification error for the following module:
module 0x1234::Test {
    fun shift(x: u64): u64 { x << 8 }
    spec shift { ensures result == x * 256 + 1; }  // The +1 should cause an error
}

But it cannot find the problem for this almost identical version:
module 0x1234::Test {
    fun shift(x: u64): u64 { x << 7 }
    spec shift { ensures result == x * 128 + 1; }  // The +1 should cause an error
}

Expected Behavior
MVP should report "post-condition does not hold" for both version of the code.
Additional context
After some investigation, I found that in move-prover/boggie-backend/src/prelude/prelude.bpl, $shl and $shr are defined to support shifting by 8, 16, 32, and 64-bits only. Is that feature on purporse or a bug?

I've submitted a PR for this bug, which I hope to be able to land very soon.
Do you expect to be using the Move Prover more in the near future? Are you affiliated with one of the blockchain projects using Move?
Thanks,
Dave

From: 五好蒜苔WHsT @.>
Sent: Thursday, March 24, 2022 5:55 PM
To: diem/move @.>
Cc: Subscribed @.***>
Subject: [diem/move] [Bug] Move Prover proves wrong spec for shifting operator (Issue #147)
🐛 Bug
Move prover proves wrong spec for shifting operator.
To reproduce
Move prover can successfully find that the verification error for the following module:
module 0x1234::Test {
fun shift(x: u64): u64 { x << 256 }

spec shift { ensures result == x * 256 + 1; }  // The +1 should cause an error

}
But it cannot find the problem for this almost identical version:
module 0x1234::Test {
fun shift(x: u64): u64 { x << 7 }

spec shift { ensures result == x * 128 + 1; }  // The +1 should cause an error

}
Expected Behavior
MVP should report "post-condition does not hold" for both version of the code.
Additional context
After some investigation, I found that in move-prover/boggie-backend/src/prelude/prelude.bpl, $shl and $shr are defined to support shifting by 8, 16, 32, and 64-bits only. Is that feature on purporse or a bug?
—
Reply to this email directly, view it on GitHubhttps://github.com/diem/move/issues/147, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABTDULBZWIHMVGP7474RPC3VBUFIDANCNFSM5RSXG4KA.
You are receiving this because you are subscribed to this thread.Message ID: @.***>

Yes, I am trying to prove the correctness of the stdlib in starcoin project.

This bug should be fixed by PR #155, which was just merged.


Testing coquelicot

Testing PR https://github.com/NixOS/nixpkgs/pull/213855

@vbgl compcert failed, can you tell if this is related?
https://github.com/coq-community/coq-nix-toolbox/actions/runs/4066426169/jobs/7002807309

I think it’s not related to this PR but rather to that one: https://github.com/NixOS/nixpkgs/pull/205646

Since upstream PR merged and nixpkgs updated since, closing this.


Errors when exporting to HTML5

I wanted to export my project to HTML5 to test it in the browser then I just noticed that the console got spammed with the error message:
SCRIPT ERROR: Invalid call. Nonexistent function 'get_status' in base 'Nil'.
at: PostgreSQLClient.poll (res://PostgreSQLClient.gd:299) - Invalid call. Nonexistent function 'get_status' in base 'Nil'.

At the beginning of the errors it says:
ERROR: Class type: 'StreamPeerSSL' is not instantiable.
   at: _new (modules/gdscript/gdscript.cpp:66) - Condition "!o" is true. Returned: Variant()
SCRIPT ERROR: Invalid call. Nonexistent function 'get_status' in base 'Nil'.
   at: PostgreSQLClient.connect_to_host (res://PostgreSQLClient.gdc:121) - Invalid call. Nonexistent function 'get_status' in base 'Nil'.

I did not touch the PostgreSQLClient script. I only ran the Helloworld.gd with modifications to auth data to connect to my database.
On desktop, it works fine.

Thank you, I did not see that post before!


Custom $lords top up doesn't work

The "Add" button stays disabled when entering the custom amount

Resolved


Disabling tab functionality

Please add the option to disable tab mode in the settings.

Tab function is core to the Notepads app and I don't see a considerable number of users wishing to disable it so I will not.


RFC: support PyUnstable_*

Do we want to expose and use these? See https://docs.python.org/3/whatsnew/3.12.html#id4 and https://docs.python.org/3.12/c-api/stable.html#unstable-c-api
For example we can have a fast path for integer conversions using PyUnstable_Long_IsCompact and PyUnstable_Long_CompactValue

I get the impression these are just a more formal naming given to what we commonly use in the cpython headers / non-abi3 builds. So if there's measurable benefit from using them I think it's fine to do so.
The only fear would be that because the cpython developers have given themselves mandate to change them between Python versions then these APIs might prove to be more volatile. I think only time will tell.

I think the conclusion is that yes, we can use these APIs, just we'll have to make sure they're tested properly on each version. Will close this issue.


Improve the most popular API references

(I suggest this issue be addressed in multiple PRs by the docs team)
We've gotten feedback that the API reference content could be better. This is also reflected in the data we collect on 👍 / 👎 , where the API reference consistently gets a worse ratio of negative to positive votes compared to the other docs. Specifically things like adding more code examples and return types could make this content better.
The following list includes (in order) the most visited API references (more than 200 views in the last 3 months). I recommend starting by going through this list in order, reviewing them and making any possible improvements.

[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.QuantumCircuit
[ ] https://docs.quantum.ibm.com/api/qiskit
[ ] https://docs.quantum.ibm.com/api/qiskit/circuit
[ ] https://docs.quantum.ibm.com/api/qiskit/circuit_library
[ ] https://docs.quantum.ibm.com/api/qiskit/transpiler
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.quantum_info.Statevector
[ ] https://docs.quantum.ibm.com/api/qiskit/quantum_info
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.RZGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.CXGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.RYGate
[ ] https://docs.quantum.ibm.com/api/qiskit/0.24/execute --> (it worries me how high on the list this page is...)
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.RXGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.Gate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.quantum_info.SparsePauliOp (got more 👎  than 👍 )
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.CPhaseGate
[ ] https://docs.quantum.ibm.com/api/qiskit/primitives
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.PhaseGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.CZGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.QFT
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.ZZFeatureMap
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.UGate
[ ] https://docs.quantum.ibm.com/api/qiskit/visualization
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.XGate
[ ] https://docs.quantum.ibm.com/api/qiskit/compiler
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.primitives.Sampler
[ ] https://docs.quantum.ibm.com/api/qiskit/0.37/qiskit.providers.aer.AerSimulator (got more 👎  than 👍 )
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.ControlledGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qasm3
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.synthesis.SuzukiTrotter
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.iSwapGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.result.Result
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.RZZGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.quantum_info.Pauli
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.HGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.U3Gate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.CCXGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.UnitaryGate
[ ] https://docs.quantum.ibm.com/api/qiskit/0.43/qiskit.circuit.QuantumCircuit
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.quantum_info.Operator
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.quantum_info.Clifford
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.transpiler.CouplingMap
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.SwapGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.primitives.Estimator
[ ] https://docs.quantum.ibm.com/api/qiskit/providers
[ ] https://docs.quantum.ibm.com/api/qiskit/pulse
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.SGate
[ ] https://docs.quantum.ibm.com/api/qiskit/circuit_classical
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.Initialize (got more 👎  than 👍 )
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.SXGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.visualization.plot_histogram
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.EfficientSU2
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.Parameter
[ ] https://docs.quantum.ibm.com/api/qiskit/providers_fake_provider
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.visualization.plot_bloch_vector
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.TwoLocal
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.quantum_info.DensityMatrix
[ ] https://docs.quantum.ibm.com/api/qiskit-ibm-runtime/qiskit_ibm_runtime.EstimatorV2
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.RXXGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.visualization.circuit_drawer
[ ] https://docs.quantum.ibm.com/api/qiskit/qasm2
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.YGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.dagcircuit.DAGCircuit
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.library.ECRGate
[ ] https://docs.quantum.ibm.com/api/qiskit/qiskit.circuit.Instruction

QuantumCircuit and qiskit.circuit recently got major overhauls - do you have any data on then pre- and post-, or should we count that as "we've looked at those two"?

Fwiw there's still probably plenty that can be done on those two, but if we're prioritising, it might make sense to pull up other things first.

yeah its quite a long list so i haven't looked through them yet to find specific improvements that could be made, if we think there are any that are already at their best we can just cross them off the list. also @abbycross and Becky might want to take a look through just for a copy edit even if theres no major updates to be made

is this issue still available?

From https://github.com/Qiskit/qiskit/issues/13297#issuecomment-2399907177:
It'd be good to look at all off the gates in qiskit/circuit/library/standard_gates at the same time as each other, and to consider what information should be in there, and what should be in the corresponding QuantumCircuit method for those gates.  It'd be good to keep that whole experience feeling fairly consistent, rather than some gates being updated and some not.
Fwiw, when we do get to that, it's probably good for a tech person and a docs person to work together on a single example gate as the first example, so we can align on everything, then the docs person can work on getting all the gates updated to the style.  I have several technical concerns with the gates API pages (mostly concerning tensor-product notation which is at best highly confusing as written), and we'd want to have that agreed before somebody goes through all of the several pages.

@DisantHoridnt: this issue is primarily intended as tracking for the IBM Quantum documentation team, but if there's any subissues that might be helpful for an external contributor to look at, one of them might comment.

got it, thanks for the reply


Fix unit testing failing issue

Purpose

The purpose of this PR is to fix #91
Goals

Goal is to fix unit testing failing issue.
Approach

Screenshots

Checklist

[x] This PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
[x] I have read and understood the development best practices guidelines ( http://bit.ly/sef-best-practices )
[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation

Related PRs

Test environment

Learning

@anjula-sack  fixed the unit test failing issue.

Removed all existing test cases.
Wrote unit tests for the service layer instead of routes.
Used mocked repositories instead of real database.


switch to using properties files to avoid maven expansion issues

Refer to this link for build results (access rights to CI server needed):
https://fabric8-ci.fusesource.com/job/fabric8-quickstarts-master-pullreq/431/
Test PASSed.


multicam example won't start L515 ?(L515 + T265)

Before opening a new issue, we wanted to provide you with some useful suggestions (Click "Preview" above for a better view):

Consider checking out SDK examples.
Have you looked in our documentations?
Is you question a frequently asked one?
Try searching our GitHub Issues (open and closed) for a similar issue.

All users are welcomed to report bugs, ask questions, suggest or request enhancements and generally feel free to open new issue, even if they haven't followed any of the suggestions above :)

Required Info

Camera Model
{L515 + T265 }

Firmware Version
(L515 <IP_ADDRESS>)

Operating System & Version
{Win (10) / Linux (Ubuntu 14/16/17)

Kernel Version (Linux Only)
(e.g. 4.14.13)

Platform
NVIDIA Jetson AGX Xavier

SDK Version
{ legacy / 2.<38>.<1> }

Language
{C/C++/opencv/pcl}

Segment
{Robot/VR/AR/others }

Issue Description
<Describe your issue / question / feature request / etc..>
I  use both T265 and L515. I can get data from L515+T265 in Win10;
But when I ported the code to Jetson AGX Xavier.
The MultiCam program that comes with the SDK can see the information.Pictures as follows:

But the source code I compiled didn't get the L515 data.I only got T265 data.Pictures as follows:

Who encountered similar problems?
Is there anyone who can solve my problem?
Thanks a lot!

Hi @jack-wlj
Have you gone through the installation process here:  https://github.com/IntelRealSense/librealsense/blob/master/doc/installation_jetson.md
How about trying the RSUSB Backend and compare it to Native Backend build?
Windows is definitely a different build/installation than the Jetson and uses different USB backend.

Hi @jack-wlj
Have you gone through the installation process here:  https://github.com/IntelRealSense/librealsense/blob/master/doc/installation_jetson.md
How about trying the RSUSB Backend and compare it to Native Backend build?
Windows is definitely a different build/installation than the Jetson and uses different USB backend.

嗨@ jack-wlj
您是否已在此处完成安装过程：https : //github.com/IntelRealSense/librealsense/blob/master/doc/installation_jetson.md
如何尝试RSUSB后端并将其与本机后端版本进行比较？
Windows绝对是不同于Jetson的构建/安装，并且使用不同的USB后端。

Thank you for your reply. I will try to compile SDK again ,and try to  building from Source using RSUSB Backend .
I will update this issue when the compilation is complete.

嗨@ jack-wlj
您是否已在此处完成安装过程：https : //github.com/IntelRealSense/librealsense/blob/master/doc/installation_jetson.md
如何尝试RSUSB后端并将其与本机后端版本进行比较？
Windows绝对是不同于Jetson的构建/安装，并且使用不同的USB后端。

Thank you for your reply. I will try to compile SDK again ,and try to  building from Source using RSUSB Backend .
I will update this issue when the compilation is complete.

嗨@ jack-wlj
您是否已在此处完成安装过程：https : //github.com/IntelRealSense/librealsense/blob/master/doc/installation_jetson.md
如何尝试RSUSB后端并将其与本机后端版本进行比较？
Windows绝对是不同于Jetson的构建/安装，并且使用不同的USB后端。

Hi,
I recompiled the code,
The same problem came up.
I  compiled the application with librealsense using   g++ -std=c++11 rs-multicam.cpp -lrealsense2 -lGL -lglut  -o rs-multicam ,
When I run the program ./rs-multicam, I can only get T265 data.I don't have data on the L515.
I tried it on two devices, the Jetson AGX  and Jetson  NX,  It's the same problem.
Why does the same rs-multicam.cpp get different results when compiled in two different ways?
So how do I compile rs-multicam.cpp code separately to get data from both T265 and L515 at the same time?
Looking forward to your reply. Thank you!

The version of the SDK I'm using is 2.41,The L515 firmware version is 1.5.3.

Hi,
We are able to build the SDK on our JETSON NANO and successfully run multicam sample with both T265 and L515 on latest repo..
If you build the entire SDK with samples enabled are you able to run the binary rs-multicam that is installed and stream? What about realsense-viewer?
Thanks

Hi,
Will you be needing further help with this?
If we don’t hear from you in 7 days, this issue will be closed.
Thanks


[Indexing options] Shredder changes to index fields

Shredder changes to use the indexing configuration to allow or deny indexing of a field

First main part resolved but need to still:

Add ITs
Fix max-string-length validation to ignore no-indexed fields.


TextInput: Weird line-height and cursor location for CJK characters

I belive this issue is straightforward. Would like to provide more info like re-pro case if necessary.

/cc @GaryQian

Hi @namiwang
testing the code sample below with the latest master
code
import 'package:flutter/material.dart';

void main() =>  runApp(MyApp());

class MyApp extends StatelessWidget {
  const MyApp({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) => MaterialApp(
    home: const MyHomePage(key: Key('home')),
  );
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) => Material(
    child: Center(
      child: TextField(
        controller: TextEditingController()..text = '你好，世界',
      ),
    ),
  );
}

doctor
[✓] Flutter (Channel master, 1.21.0-2.0.pre.94, on Mac OS X 10.15.5 19F101, locale en-GB)
    • Flutter version 1.21.0-2.0.pre.94 at /Users/nevercode/development/flutter_master
    • Framework revision adc9dde3ba (8 hours ago), 2020-07-21 16:17:23 -0700
    • Engine revision 5a73d4dc25
    • Dart version 2.9.0 (build 2.9.0-21.0.dev 84e5eeaea4)

[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
    • Android SDK at /Users/nevercode/Library/Android/sdk
    • Platform android-29, build-tools 29.0.2
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 11.5)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 11.5, Build version 11E608c
    • CocoaPods version 1.9.0

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 4.0)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin version 46.0.2
    • Dart plugin version 193.7361
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)

[✓] VS Code
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.12.2

[✓] Connected device (3 available)
    • macOS (desktop)  • macos      • darwin-x64     • Mac OS X 10.15.5 19F101
    • Web Server (web) • web-server • web-javascript • Flutter Tools
    • Chrome (web)     • chrome     • web-javascript • Google Chrome 84.0.4147.89

• No issues found!

the issue seems to be fixed; I feel safe to close this issue,
if you disagree please write in the comments and I will reopen it.
Thank you


Feature/ds ml service one docker file and readme update

closes #272
adds info into README as requested in #266

Nice work @gerardcl

thanks, also credits to @hugowschneider and @sklingel ! :)


Link Operator Readme to the HotelReservation/Kubernetes tutorial

Modify the section on Kubernetes operators in the Kubernetes tutorial readme to link to the Operator repo/branch.  Include a high-level description.   The idea is for the Kubernetes tutorial to be the entry point to learning about Kubernetes.

Just making sure the k8s tutorial you're referring to these two: instructions inside profile repo and tutorial summary inside hacking-exercise repo

Yes.  Though both of your hyperlinks above point to the same URL :).   The hacking-exercise repo's kubernetes operator sectoin should simply point to the "instructoins inside profile repo of operator branch". Make sense?

Oops, the first link I was referring to readme in dsb_k8s repo

It turns out I don't have permission to write the tutorial repo. @RS1999ent @MaxLanLiu Could you guys add me as a collaborator?


Hide node status background when there is no status to show

Fixes #4418
Ensures the node status background is hidden when there's nothing to show.

Tested  - working. ✅