Removes test_range_bearing

As pointed out by @hidmic in #185, https://github.com/osrf/lrauv/pull/174 makes test_range_bearing redundant. This PR eleminates the test and the associated files.
Signed-off-by: Arjo Chakravarty<EMAIL_ADDRESS>

I agree with you won't merge yet. I'm quite surprised by the regressions.

Don't see any harm in merging this now. We will have to revisit the dynamics tests.


Fix NPE found with list constructor var ref

Purpose
$title
Fixes #20532
Approach
n/a
Samples
Now allows;
    var fooList = [
        ["AAA", 111],
        ["BBB", 222]
    ];

    foreach var foo in fooList {}

    var barList = ["CCC", 333];

    (int|string)[] fooArr = ["DDD", 444];

Remarks
n/a
Check List

[x] Read the Contributing Guide
[ ] Updated Change Log
[ ] Checked Tooling Support (#)
[x] Added necessary tests

[x] Unit Tests
[ ] Spec Conformance Tests
[ ] Integration Tests
[ ] Ballerina By Example Tests

[ ] Increased Test Coverage
[ ] Added necessary documentation

[ ] API documentation
[ ] Module documentation in Module.md files
[ ] Ballerina By Examples

Codecov Report

Merging #20564 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #20564   +/-   ##
=======================================
  Coverage   15.21%   15.21%
=======================================
  Files          50       50
  Lines        1341     1341
  Branches      208      208
=======================================
  Hits          204      204
  Misses       1121     1121
  Partials       16       16

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9573b9...f281213. Read the comment docs.


curl to follow 301 redirects; repo changed

Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@wwwehr thanks for the PR! the Dockerfiles that are being actively built and maintained have been moved to https://github.com/aws/deep-learning-containers/tree/master/pytorch/inference/docker - could you open up a PR there instead?


AES-CBC writes past the end of buffer when decrypting

There is a bug with AES-CBC in 1.1.1.  I've confirmed the bug is present with the no-asm config option.
The problem was reported in x86 32 bit mode with assembly.  The platform (given to Configure) was: “linux-elf”.
It does not seem to occur for x86/64.  I also couldn't reproduce it on master.
The report is:

Say you have a memory buffer into which you are decrypting data in fixed-sized "chunks" using BIO_read() until EOF (e.g. until BIO_read() returns no more data). Also – knowing the exact amount of data to be decrypted – the buffer being decrypted to is exactly of that size. In such a scenario it seems like that in 1.1.x in certain cases an extra block might be accessed past the end of the passed in buffer (see attached code example demonstrating this issue).
In the past (i.e. OpenSSL versions before 1.1.x), the BIO_read() decryption would not access the memory of the passed in buffer past the size of the decrypted data regardless of the requested read length (even if during the decryption an extra block was used), I presume because an internal buffer was used (which accommodated the extra block as needed). Hence the attached example code works fine when linked with older OpenSSL releases. However in 1.1.x (I’m guessing since the "perform enc_read operation without using overlapping buffers" change was made), the caller passed in buffer seems to be used directly and as such this buffer overflow scenario could occur (e.g. in programs that expect the old behavior).

 Which is correct?

It's CBC 😢
I've fixed the title and description.

So...  this depends on the config target, you said?  linux-x86 behaves correctly while linux-elf doesn't?  I assume that this is built on a x86_64 machine...
Something to be noted is that linux-x86 adds -m32 among the C flags, while linux-elf doesn't.  That's literally the only difference between them.  Could this be of importance, i.e. if you configure with ./Configure linux-elf -m32, things are working properly?
Something to be noted is that linux-elf is aged.  There's a comment regarding this in Configurations/10-main.conf:
    #### IA-32 targets...
    #### These two targets are a bit aged and are to be used on older Linux
    #### machines where gcc doesn't understand -m32 and -m64
    "linux-elf" => {
...
    },
    "linux-aout" => {
...
    },

(this makes me think we need a deprecation mechanism for config targets...  these aged ones should be retired)

I reproduced this on an x86/64 machine running Ubuntu 18.04 by configuring with ./config no-asm.

I reproduced this on an x86/64 machine running Ubuntu 18.04 by configuring with ./config no-asm.

That should become a linux-x86_64 config target, not linux-elf

To repeat myself...  is there a difference if you:

./Configure linux-elf no-asm -32
./Configure linux-x86 no-asm

(can you please tell me how you compile your test program?  ASan is acting up on me)

From memory (on a different machine): gcc-9 -o bug bug.c --fsanitize-address -lcrypto
Plus -I and -L options to find includes and libraries.
I built OpenSSL with gcc-9 and address sanitisation:  CC=gcc-9 ./config debug --fsanitize-address (or something close).

Interestingly enough, I can't reproduce this on Debian...

I'll do a few more tests

...  but maybe if I did my testing in the 1.1.1 branch instead of master :laughing:

Confirmed to bug with the following configurations:

linux-elf no-asm
linux-elf no-asm -m32
linux-x86 no-asm
linux-x86_64 no-asm (!!!!!)

It's safe to say that this isn't a configuration problem

I don't think the no-asm is necessary, I believe the problem occurs with -m32 alone.
My guess is that the fault is in the CBC mode code (x86/64 has asm for CBC, x86 doesn't and of course no-asm doesn't).

I believe the problem occurs with -m32 alone.

Not according to my tests, where even x86_64 fails

Hmmm. A "stab in the dark" theory. EVP_DecryptUpdate is actually documented as possibly writing more data than you might otherwise expect:

EVP_DecryptInit_ex(), EVP_DecryptUpdate() and EVP_DecryptFinal_ex() are the
corresponding decryption operations. EVP_DecryptFinal() will return an
error code if padding is enabled and the final block is not correctly
formatted. The parameters and restrictions are identical to the encryption
operations except that if padding is enabled the decrypted data buffer B
passed to EVP_DecryptUpdate() should have sufficient room for
(B + cipher_block_size) bytes unless the cipher block size is 1 in
which case B bytes is sufficient.

Note that the output buffer length is an output parameter only. So you can't specify how long your buffer is. It's just assumed that you've given it enough space. Interestingly though the cipher BIO does seem to take account of this and ensure there is enough space for that extra block:
https://github.com/openssl/openssl/blob/3478a2102e8bfc63f20e22826ab39b407ff43f62/crypto/evp/bio_enc.c#L172-L184
So, if this is the cause of the problem, its not too clear where it might be going wrong.

I believe the problem occurs with -m32 alone.
Not according to my tests, where even x86_64 fails

x86_64 including asm or no-asm?
-m32 is supposed to fail with asm.

Does CBC mode need padding when the block is a multiple of block size?  800 / 16 = 50.
That it is split into a 500 and a 300 byte read does weird things to the block alignment.

Might be worth trying linux-elf -m32 as well.

Does CBC mode need padding when the block is a multiple of block size?

You always have at least one byte of padding. So if the plaintext is a multiple of the block size then you'll get a whole block of padding.

That's a smoking gun.  I also knew that but didn't -- memory in superposition.
I'm going to go to sleep.

The issue isn't with encryption, it's with decryption, and the result of the decryption should be exactly the same as the same as the original plaintext.
Note that both source_buf and dst_buf are allocated to the same size, BUF_SIZE (800 bytes).  The ASAN output is this:

==30999==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6180000007a0 at pc 0x7fbdca759067 bp 0x7fff696b6040 sp 0x7fff696b6038
...
0x6180000007a0 is located 0 bytes to the right of 800-byte region [0x618000000480,0x6180000007a0)

0x6180000007a0 is dst_buf + 800

So here's an interesting comment...
https://github.com/openssl/openssl/blob/3478a2102e8bfc63f20e22826ab39b407ff43f62/crypto/evp/bio_enc.c#L172-L176

Note that bio_enc.c hasn't changed between 1.1.1 and master, so I have no idea for the moment why we don't get the same failure in the latter...

Here's an interesting comment...

Didn't I just post that same code snippet!? :-)

Ah, right, you did...

Buuuut, that's a red herring, line 177 should prevent the padding block to be decrypted directly into the output buffer...

Ah!  Of course!  outl is much larger than i, so even though outl - blocksize appears to fix things, it doesn't, because it's really the fault of the test program, which intentionally asks for a reading size much larger than what's available, so even if enc_read has only read x bytes, where x is much smaller than outl, it's going to try and decrypt everything, including the padding block, right into out.  [BAMF]
And it makes sense, really.  The test program basically does this:
BIO_read(bio, dst, 500);
BIO_read(bio, dst + 500, 500);

... and this, with a dst that has been allocated 800 bytes.
If you did the same with a POSIX read(), the natural response would be user error.  It's the same here.

If I change BUF_READ_CHUNK to 408 (exactly half of the encryption size, 816), then the test program passes with no errors

A more properly written test (even though a bit ugly):
#include <openssl/aes.h>
#include <openssl/bio.h>
#include <openssl/evp.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

#define BUF_SIZE 800
#define BUF_READ_CHUNK 500

int main() {
    // Encryption
    unsigned char *source_buf = calloc(1, BUF_SIZE);
    BIO *bio_encrypt = BIO_new(BIO_f_cipher());
    BIO *bio_write_mem = BIO_new(BIO_s_mem());
    unsigned char key[16] = { 0 };
    unsigned char iv[16] = { 0 };

    BIO_push(bio_encrypt, bio_write_mem);
    BIO_set_cipher(bio_encrypt, EVP_aes_128_cbc(), key, iv, 1);
    BIO_write(bio_encrypt, source_buf, BUF_SIZE);
    BIO_flush(bio_encrypt);

    const void *encrypted_data;
    int encrypted_data_len = BIO_get_mem_data(bio_write_mem, &encrypted_data);

    // Decryption
    unsigned char *dst_buf = malloc(BUF_SIZE);
    memset(dst_buf, 1, BUF_SIZE);
    BIO *bio_decrypt = BIO_new(BIO_f_cipher());
    BIO *bio_read_mem = BIO_new_mem_buf(encrypted_data, encrypted_data_len);
    BIO_push(bio_decrypt, bio_read_mem);
    BIO_set_cipher(bio_decrypt, EVP_aes_128_cbc(), key, iv, 0);
    int total_read = 0;
    int to_read, read;
    for (read = 0;
         ((to_read = total_read + BUF_READ_CHUNK > BUF_SIZE
           ? BUF_SIZE - total_read : BUF_READ_CHUNK),
          (read = BIO_read(bio_decrypt, dst_buf+total_read, to_read)) > 0);
         total_read += read)
      printf("read: %d\n", read), fflush(stdout);

    printf("memcmp: %d\n", memcmp(source_buf, dst_buf, BUF_SIZE));

    BIO_free(bio_encrypt);
    BIO_free(bio_decrypt);
    BIO_free(bio_write_mem);
    BIO_free(bio_read_mem);
    free(source_buf);
    free(dst_buf);
    return 0;
}

The interesting bit now is why the test program before my mod doesn't fail on master...

The answer to that question is here:
https://github.com/openssl/openssl/blob/3478a2102e8bfc63f20e22826ab39b407ff43f62/providers/common/ciphers/cipher_common.c#L179-L183
In other words, the buffering that was once in BIO_f_cipher's reading function has now been re-implemented in the implementations.  As long as the output buffer matches the plaintext size, there will be no writing outside of the allocated space, even though the size given to BIO_read pretends something else.
Doing something similar in 1.1.1 would mean doing changes in the EVP_CIPHER implementations, and in the worst case, change them quite radically (I haven't entirely grasped how do_cipher is called in the update step vs the final step, and if that kind of call requires some extra cipher flags, nor what the consequences of such changes would be).

Is this all enough to close this case?

Is this all enough to close this case?

Looks it to me. But lets wait for @paulidale to comment.

I'm the originator of this issue and I was not sure if it was to be considered a bug (hence my contacting @paulidale about it)
In my original email I added to my demonstration/analysis that (given that such an "overread" in BIO_read() was not causing issues in earlier OpenSSL releases):

"... As such this is what I would consider a possible "backwards compatibility" issue. So I was wondering if this change how the API behaves is expected/intentional? (If yes, I guess users of OpenSSL – if affected – would need to adjust their logic accordingly)."

As I understand this seems to be the consensus here now (at least for 1.1.1, as for later (master-based?) releases the "issue" is addressed in OpenSSL itself).

Oh, and I could reproduce the issue with the following config (on 1.1.1d):

linux-x86 -m32

As I understand this seems to be the consensus here now

Right. You were lucky that this worked before. It was an accident of the earlier implementations that you didn't run into this bug. Things got moved around and work slightly differently - and suddenly you're hitting the problem, i.e. this is not considered an OpenSSL bug.

Do note that with OpenSSL 3.0, you'll get "lucky" again :wink:

I'm fine to close this.  Thanks for looking into it.


Update linter config

Fixes linter warnings
Updates the linter config

Closes #132
Depends on #256

I addressed the comments. There are two points for discussion remaining:

Do we want to forbid functions to start with Get…?
Enabling revive (formerly golint) reports:

exported: func name will be used as test.TestAddress by other packages, and that stutters; consider calling this Address
exported: func name will be used as test.TestAccountWithWalletAndBackend by other packages, and that stutters; consider calling this AccountWithWalletAndBackend

I think this is the reason why we normally named the generic tests with Generic…Test.  You can see the reported errors here.
@matthiasgeihs

Do we want to forbid functions to start with Get…?

Typically we shouldn't use "Get...". I had a look and it seems like there are easy work arounds for the locations where we are using it. In many cases we can simply remove it.

Enabling revive (formerly golint) reports:

exported: func name will be used as test.TestAddress by other packages, and that stutters; consider calling this Address
exported: func name will be used as test.TestAccountWithWalletAndBackend by other packages, and that stutters; consider calling this AccountWithWalletAndBackend

I think this is the reason why we normally named the generic tests with Generic…Test.

I see. However, I don't think "Generic...Test" is a good name (or idiomatic) either. I don't have the perfect solution for this. I see two options:

Either we surpress the linter warning for the respective cases.
We go back to the other naming schema.

I have a preference for 1, but as I said, it is also not perfect. Any other suggestions?

Typically we shouldn't use "Get...". I had a look and it seems like there are easy work arounds for the locations where we are using it. In many cases we can simply remove it.

Yes i think so as well.

I have a preference for 1, but as I said, it is also not perfect. Any other suggestions?

I dont mind, but i also dont have a better naming scheme :laughing: will suppress for now.

All addressed. Changes are in the last force push.
@matthiasgeihs

It looks like i messed up some timeout, since a follow up branch for this one fails in the CI…

It looks like i messed up some timeout, since a follow up branch for this one fails in the CI…

Could not reproduce now after ~8 runs, maybe was false alarm with slow runners.
The run in question was this.

It looks like i messed up some timeout, since a follow up branch for this one fails in the CI…

Could not reproduce now after ~8 runs, maybe was false alarm with slow runners. The run in question was this.

Might have been related to the context changes?


OpenStackWatcher needs to set account_type

Addresses auditor load issue (from @jleaniz) and adding account_type to OpenStackWatcher

Coverage decreased (-0.01%) to 70.724% when pulling 10ade0f84244a002734f8ca934a100b3ee56bebf on mstair:issue_860_openstackwatcher into d5e503e2fb3cd29d4aa9fb48ff97b2fed024a826 on Netflix:develop.

Coverage decreased (-0.01%) to 70.724% when pulling 79266f464825a23a27037fcb0eac181c454ffe4c on mstair:issue_860_openstackwatcher into d5e503e2fb3cd29d4aa9fb48ff97b2fed024a826 on Netflix:develop.


feat: Add the prefix schema to the name of the document symbol

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

[ ] N
[x] Y  fix #1560

2. What is the scope of this PR (e.g. component or file name):
kclvm/tools/src/LSP/src/document_symbol.rs

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
Add the prefix schema to the name of the document symbol. Avoid new a document symbol with empty string ""
https://github.com/microsoft/vscode/blob/922413f6d97e16c05b565398f33d95c306b1ceb7/src/vs/workbench/api/common/extHostTypes.ts#L1384

[ ] Affects user behaviors
[ ] Contains syntax changes
[ ] Contains variable changes
[ ] Contains experimental features
[ ] Performance regression: Consumes more CPU
[ ] Performance regression: Consumes more Memory
[x] Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

[x] N
[ ] Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

[x] Unit test
[ ] Integration test
[ ] Benchmark (add benchmark stats below)
[ ] Manual test (add detailed scripts or steps below)
[ ] Other

Pull Request Test Coverage Report for Build<PHONE_NUMBER>5
Details

5 of 5 (100.0%)  changed or added relevant lines in 1 file are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.002%) to 70.525%

Files with Coverage Reduction
New Missed Lines
%

kclvm/tools/src/LSP/src/inlay_hints.rs
3
92.31%

Totals

Change from base Build<PHONE_NUMBER>6:
-0.002%

Covered Lines:
50429

Relevant Lines:
71505

💛  - Coveralls


Update to use embed package for bundling sources specification

https://pkg.go.dev/embed

Done as of v0.2.0


framework/workload: ApkWorkload clear_data option

When set to true, the data associated with the package will be cleared
(pm clear), before each workload run. The default value is "true" and therefore,
WA behavior remains the same. However, it gives a way of bypassing this
operation which is interesting for a workload such as PCMark, which wouldn't
have to download the benchmark data-set before each run. This download is
time-consuming and pollutes the benchmark instrumentation.

Why not just set clear_data_on_reset to False inside PCMark workload? Why does this need to be configurable by the user?

I originally didn't want to change the current behavior. But if you think it is alright to modify it, I can enforce clear_data_on_reset to false only for PCMark, without any option.

Well, you'd only be changing the behaviour for PCMark, and whole point is that it needs it. Since this is tied to the workload, and no the user's setup (i.e. it always makes sense for PCMark), this should be part of the workload's implementation, rather than forcing the user to have to specify the parameter each time.


Allow specific php files ?

Hey guys !
Currently expecting some issues with a file. I've created a php file for example named test.php.
If I try to open it on site, I'm getting "Cannot GET /test.php"
I think I have to edit something in Index.js, but I can't get it to work actually.
Pretty new to vuejs :/
Would be cool if someone could help me with this !
Thanks in advance !

What do you mean, why would you want to create a /test.php file?
The script is not even running on php.

The php file shows the bot online status. Yes I know but shouldn't it be possible ?
Otherwise do you know a way to get the online status of a steam account in js ?

Just use Steam's API, it  provides the personastate of a user. https://developer.valvesoftware.com/wiki/Steam_Web_API#GetPlayerSummaries_.28v0002.29

A module installed with this script can already give you the data that you need.
https://github.com/DoctorMcKay/node-steamcommunity/wiki/CSteamUser

Also, you can't run php scripts with node afaik. Use a reverse proxy.


Fixed the signup button color in the frontend

From this

To this

Thanks @DUMBANIKET, but it's resolved now.


Definition of high-quality

There is no clear definition what high-quality means in this repo.

Great point, let's define this precisely. Will circle back soon to do so.

App Mining funds non-working apps because they have "fancy" landing pages. We have introduced on the Blockstack.org, and we got $2 this month. What an investment huh? Seems like we will out app mining soon, even Blockstack.

Sorry about the super small payout, I'm sure that is frustrating. We're considering some alterations to the payout plan which might help there.
Also, if you feel like the reviewers are rewarding the wrong stuff, feel free to propose new criteria and reviewers, that is what the repo is for.

@friedger is there a specific problem or change you're referencing? On the readme?

We can add a section to the readme under ecosystem activity to be encouraged that highlights qualities of apps that would fit and perform well in App Mining.


Notifications desktop/mobile updates

https://github.com/AugurProject/augur/issues/1484
https://github.com/AugurProject/augur/issues/1500
https://github.com/AugurProject/augur/issues/1501
https://github.com/AugurProject/augur/issues/1460

good call using QuadBox @phoebemirman, lots of duplicated code removed 💥
also regarding the "red dot disappear", that will be addressed in https://github.com/AugurProject/augur/issues/1517.
As of now, you need to click the action buttons to make the notifications not appear as "NEW".


401 errors are not ignored in any way

Hello, I have a problem.
(I already read it https://github.com/microsoft/ApplicationInsights-node.js#preprocess-data-with-telemetry-processors)
const appInsights = require('applicationinsights');

const clearErrors = (envelope, context) => {
  const {
    data: {
      baseData: { responseCode, resultCode }
    }
  } = envelope || { data: { baseData: {} } };

  const { statusCode, statusMessage } = context['http.ServerResponse'] || {};

  if (
    responseCode === 401 ||
    statusCode === 401 ||
    resultCode === '401' ||
    statusMessage === 'Unauthorized'
  ) {
    return false;
  }

  return true;
};

appInsights.setup().start();

appInsights.defaultClient.addTelemetryProcessor(clearErrors);

And usage:
function errHandler(err, req, res, next) {
  const status = error.status || error.statusCode;

  if (status < 500) {
    if (status !== 401) {
      logger.debug(err, 'Error');
    }
  } else {
    telemetry.trackException({ exception: err });
  }
}

...
const app = express();

app.use(errHandler);

But if I make a request to my API, where I check for authorization, and if we are not authorized, I throw
const { Unauthorized } = require('http-errors');
...
new Unauthorized()

Locally if I get 401 errors then they are not sent to applicationInsights (My filter sees responseCode === 401 || statusCode === 401 ||  resultCode === '401' || statusMessage === 'Unauthorized' and ignores).
But if I host the code in Azure. Then in my applicationInsights I see a lot of 401 errors, they are still being logged.

According to docs, the azure auto-instrumentation will be disabled if AI SDK is found and running into the node.js app.

If both auto-instrumentation monitoring and manual SDK-based instrumentation are detected, only the manual instrumentation settings will be honored. This is to prevent duplicate data from being sent. To learn more about this, check out the troubleshooting section below. Azure Docs Source

In this case, I would recommend turn off auto-intrumentation (node in-processs) Application Insights, and let only the SDK enabled on app level.


Migrate sidebars.js to code repo and add describe

This path migrate sidebars.js to our main repo and
and describe about how our tool/build_docs.sh work.
ref: apache/incubator-seatunnel#1586

Thanks


Fix warning: more use of == in void context

@jstirk I modified a problem on the basis of your comments (mentioned the below url), and updated.
Please check it !
https://github.com/kennyj/mail/commit/32512916e788454be8edee0bcd7dbe26aead235a#commitcomment-867284


Update stubs to generate anonymous classes

This PR updates the generated code to align with Laravel’s current approach for migrations. Laravel has long adopted anonymous classes for migrations to prevent class name collisions. With this change, you can now generate migrations using:
php artisan make:patch update_users
php artisan make:patch update_users

The generated patches are now cleaner, with unnecessary long comments removed, as documentation is a more appropriate place for them.
The library still supports the older patch structure for those who already have them in their projects.
Additionally, this PR removes support for PHP 8.0, which reached its end of life.

@rappasoft , is this package maintained or abandoned? No activity on the repository for a long time.

@rappasoft ? Can we please proceed with if there is no issue?


Revert "Migrate policies and collaborator roles from delegate access"

Reverts ministryofjustice/modernisation-platform#6191

There is a circular dependency between jobs. I need to manually stop the workflow once delegate access is successful because other jobs are deleting the resources created during delegate access. This is due to the revert operation.


A simpler way to force a view to get back the initial collection type

Currently, if a user takes as parameter a List[Int] and get a View from it, there is no automatic way to get back a List, unless the user explicitly mentions the companion object: xsView.to(List). I wish we could just write xs.force(), so that if we change the type of the underlying collection to be a Vector we don’t have to repeat the change when calling .to(Vector).
#144 Provides a solution to this problem by introducing the concept of view transformers. However it only works with unsorted collection types and with collection type constructors of kind * -> *. We should investigate the ability to also support sorted collections and other kinds.

IMHO this is unnecessary. Do you ever care about getting the same type as the source collection back without knowing what it was? Either you know what you start with or you know what you want to build no matter what.
(If we should end up doing this https://github.com/scala/collection-strawman/issues/543 needs to be reopened and fixed properly)

@szeiger - Very often the choice of collection is made for performance reasons.  Having it be maintained without knowing what it was is usually the performance-friendly option; eager operations on Seq have this property.  Losing this property for views is okay, but it is a loss.  (We already deal with this loss with Iterator, for instance.)

Losing this property for views is okay, but it is a loss

I take your point, but there is a sense in which it also a gain. No longer carrying this information makes it easier to understand what views are and how they behave.

@SethTisue - I agree.  My point was only that it's a tradeoff.  I don't have a strong opinion about which consideration is more important.  Since it is work to make it remember the information, I would lean more towards "forget the underlying type".

looks like this isn't happening.


Unbreak shadows by retaining work item buffers corresponding to ExtractedViews, not ViewTargets.

OK, so this is tricky. Every frame, delete_old_work_item_buffers deletes the mesh preprocessing index buffers (a.k.a. work item buffers) for views that don't have ViewTargets. This was always wrong for shadow map views, as shadow maps only have ExtractedView components, not ViewTargets. However, before #16836, the problem was masked, because uploading the mesh preprocessing index buffers for shadow views had already completed by the time delete_old_work_item_buffers ran. But PR #16836 moved delete_old_work_item_buffers from the ManageViews phase to PrepareResources, which runs before write_batched_instance_buffers uploads the work item buffers to the GPU.
This itself isn't wrong, but it exposed the bug, because now it's possible for work item buffers to get deleted before they're uploaded in write_batched_instance_buffers. This is actually intermittent! It's possible for the old work item buffers to get deleted, and then recreated in batch_and_prepare_binned_render_phase, which runs during PrepareResources as well, and under that system ordering, there will be no problem other than a little inefficiency arising from recreating the buffers every frame. But, if
delete_old_work_item_buffers runs after
batch_and_prepare_render_phase, then the work item buffers corresponding to shadow views will get deleted, and then the shadows will disappear.
The fact that this is racy is what made it look like #16922 solved the issue. In fact, it didn't: it just perturbed the ordering on the build bots enough that the issue stopped appearing. However, on my system, the shadows still don't appear with #16922.
This commit solves the problem by making delete_old_work_item_buffers look at ExtractedViews, not ViewTargets, preventing work item buffers corresponding to live shadow map views from being deleted.

A rendering PR I actually understand!
LGTM

I know, right?! 😆


Prefer using qemu system instead of qemu user

s390x-unknown-linux-gnu and sparc64-unknown-linux-gnu uses qemu system to run the tests. It seems more correct in terms of emulation to use full system emulation. So we may try to update the other targets to use system emulation.

This was added for s390x and sparc64, but other platforms aren't using this yet. Changing the others, though, to using this would be more than welcome!

Status update: @malbarbo added qemu-system support to cross - most targets can choose between using qemu-user or qemu-system when running tests.

Something to note here is that qemu-system seems to be significantly slower than qemu-user. Most tests complete in ~3 minutes, but s390x is about 12m and sparc64 is about 25.
Checking tier 1 and maybe some of the more popular tier 2 targets with qemu-system may be worth it for accuracy and CI could be adjusted so they don't block anything (currently docker tier 2 blocks the linux build channels), but it doesn't seem like it would be worth the slowdown for many more targets.


Fix run overload bug for spawned process

Codecov Report

Merging #89 (bb02852) into master (5d7e023) will decrease coverage by 2.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   99.79%   97.51%   -2.28%
==========================================
  Files          19       19
  Lines        1961     1933      -28
==========================================
- Hits         1957     1885      -72
- Misses          4       48      +44

Impacted Files
Coverage Δ

src/viztracer/patch.py
84.76% <0.00%> (-15.24%)
:arrow_down:

src/viztracer/report_builder.py
94.59% <0.00%> (-5.41%)
:arrow_down:

src/viztracer/viztracer.py
94.61% <0.00%> (-5.39%)
:arrow_down:

src/viztracer/simulator.py
96.77% <0.00%> (-3.23%)
:arrow_down:

src/viztracer/flamegraph.py
97.59% <0.00%> (-2.41%)
:arrow_down:

src/viztracer/code_monkey.py
97.95% <0.00%> (-2.05%)
:arrow_down:

src/viztracer/main.py
98.81% <0.00%> (-1.19%)
:arrow_down:

src/viztracer/prog_snapshot.py
99.44% <0.00%> (-0.56%)
:arrow_down:

src/viztracer/tracer.py
100.00% <0.00%> (ø)

src/viztracer/decorator.py
100.00% <0.00%> (ø)

... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d7e023...bb02852. Read the comment docs.

Codecov Report

Merging #89 (bb02852) into master (5d7e023) will decrease coverage by 2.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   99.79%   97.51%   -2.28%
==========================================
  Files          19       19
  Lines        1961     1933      -28
==========================================
- Hits         1957     1885      -72
- Misses          4       48      +44

Impacted Files
Coverage Δ

src/viztracer/patch.py
84.76% <0.00%> (-15.24%)
:arrow_down:

src/viztracer/report_builder.py
94.59% <0.00%> (-5.41%)
:arrow_down:

src/viztracer/viztracer.py
94.61% <0.00%> (-5.39%)
:arrow_down:

src/viztracer/simulator.py
96.77% <0.00%> (-3.23%)
:arrow_down:

src/viztracer/flamegraph.py
97.59% <0.00%> (-2.41%)
:arrow_down:

src/viztracer/code_monkey.py
97.95% <0.00%> (-2.05%)
:arrow_down:

src/viztracer/main.py
98.81% <0.00%> (-1.19%)
:arrow_down:

src/viztracer/prog_snapshot.py
99.44% <0.00%> (-0.56%)
:arrow_down:

src/viztracer/tracer.py
100.00% <0.00%> (ø)

src/viztracer/decorator.py
100.00% <0.00%> (ø)

... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d7e023...bb02852. Read the comment docs.