Incorrect Routing of Upstream Packets (Two clients and one game server)

What happened:
Hi all,
We've observed an issue that a proxy pod uses the same socket for traffic from different clients to the same game server, resulting in one of the clients not receiving response from the game server? Have any specific edge cases been identified as causing this issue?
[Our architecture setup]
In the game server kubernetes cluster, we have a Load Balancer that routes to multiple proxy pods (not as sidecars) and control planes with the Agones provider. We’re using the same token for both clients.

What you expected to happen:
We expect two clients can receive corresponding responses.

How to reproduce it (as minimally and precisely as possible):
Unknown. Once it started to occur at a some point, it started happening intermittently throughout the day. We suspect there may be a buggy state in a specific pod instance.
Anything else we need to know?:
Environment:

Quilkin version:
v0.8.0

Execution environment (binary, container, etc):
kubernetes, container

│ Containers:                                                                                │
│   quilkin:                                                                                │
│     Container ID:  containerd://9975e4e955b7102297415506422e3c1ebd5b4c39a61bd5039656807e5ae4a1a7                              │
│     Image:         us-docker.pkg.dev/quilkin/release/quilkin:0.8.0                                                            │
│     Image ID:      us-docker.pkg.dev/quilkin/release/quilkin@sha256:3f0abe1af9bc378b16af84f7497f273caaf79308fd04ca302974d070f │
│ e68b8e2

Operating system:

Custom filters? (Yes/No - if so, what do they do?):

version: v1alpha1                                                                                │
│ filters:                                                                                │
│   - name: quilkin.filters.capture.v1alpha1.Capture                                                                            │
│     config:                                                                                │
│       suffix:                                                                                │
│         size: 7                                                                                │
│         remove: true                                                                                │
│   - name: quilkin.filters.token_router.v1alpha1.TokenRouter

Log(s):
Others:

Thank you for your issue! Would you mind testing one of the latest images and see if you can reproduce? Just to eliminate the possibility that it has already been fixed.

Thank you for your issue! Would you mind testing one of the latest images and see if you can reproduce? Just to eliminate the possibility that it has already been fixed.

@XAMPPRocky Thanks for your reply. We would appreciate it if there is a latest image available.us-docker.pkg.dev/quilkin/release/quilkin:0.9.0-dev-50d91e4
Or is there guidance document for building custom image?

You can also grab from one of our PR builds, e.g https://github.com/googleforgames/quilkin/pull/987#issuecomment-2209254047

Thank you for your issue! Would you mind testing one of the latest images and see if you can reproduce? Just to eliminate the possibility that it has already been fixed.

@XAMPPRocky  We have tried using the latest image, but unfortunately, the issue persists. Additionally, the CPU and memory usage are much higher than in version v0.8.0. 🥲

@zezhehh That is odd, because we have used and tested this setup of having one token per gameserver with multiple clients on a single proxy and haven't had an issue at all. Would you be able to check your load balancer setup that you're running infront of the proxy, my first reckon is with that as we don't put any load balancers in front of the proxies, so that's one difference I see with the setup we have tested.

@zezhehh can you share what kind of LB it is? (i.e. is it a Google Cloud / AWS LoadBalancer? How is it configured etc?) Maybe there is something in there.

@XAMPPRocky @markmandel
We have a GCP k8s LB setting as follows, which should preserve the original source IP:port.
│ Name:                     quilkin-proxy                                                                                │
│ Namespace:                quilkin                                                                                │
│ Labels:                   app.kubernetes.io/managed-by=Terraform                                                                                │
│ Annotations:              cloud.google.com/l4-rbs: enabled                                                                                │
│                           cloud.google.com/neg: {"exposed_ports": {"7337":{},"7338":{}}}                                                                                │
│                           cloud.google.com/neg-status:                                                                                │
│                             {"network_endpoint_groups":{"7337":"k8s1-c8523907-quilkin-quilkin-proxy-7337-c77ae3d6","7338":"k8s1-c8523907-quilkin-quilkin-proxy-7338-c0...           │
│                           service.kubernetes.io/backend-service: k8s2-e2o2llay-quilkin-quilkin-proxy-6p9r6aaw                                                                       │
│                           service.kubernetes.io/firewall-rule: k8s2-e2o2llay-quilkin-quilkin-proxy-6p9r6aaw                                                                         │
│                           service.kubernetes.io/firewall-rule-for-hc: k8s2-e2o2llay-quilkin-quilkin-proxy-6p9r6aaw-fw                                                               │
│                           service.kubernetes.io/healthcheck: k8s2-e2o2llay-quilkin-quilkin-proxy-6p9r6aaw                                                                           │
│                           service.kubernetes.io/udp-forwarding-rule: a6675171c0d494944ac00781b235adf0                                                                               │
│ Selector:                 role=proxy                                                                                │
│ Type:                     LoadBalancer                                                                                │
│ IP Family Policy:         SingleStack                                                                                │
│ IP Families:              IPv4                                                                                │
│ IP:                       <IP_ADDRESS>                                                                                │
│ IPs:                      <IP_ADDRESS>                                                                                │
│ IP:                       <IP_ADDRESS>                                                                                │
│ LoadBalancer Ingress:     <IP_ADDRESS>                                                                                │
│ Port:                     proxy-udp  7337/UDP                                                                                │
│ TargetPort:               proxy-udp/UDP                                                                                │
│ NodePort:                 proxy-udp  30118/UDP                                                                                │
│ Endpoints:                <IP_ADDRESS>:7777,<IP_ADDRESS>:7777,<IP_ADDRESS>:7777                                                                                │
│ Port:                     ping-udp  7338/UDP                                                                                │
│ TargetPort:               ping-udp/UDP                                                                                │
│ NodePort:                 ping-udp  31821/UDP                                                                                │
│ Endpoints:                <IP_ADDRESS>:7600,<IP_ADDRESS>:7600,<IP_ADDRESS>:7600                                                                                │
│ Session Affinity:         None                                                                                │
│ External Traffic Policy:  Local                                                                                │
│ HealthCheck NodePort:     30737

The issue has been confirmed by examining dumped UDP traffic in the pcap file, which can be viewed using Wireshark.
Quilkin Proxy Capture.pcap.zip
The file contains data from two ongoing games.
The Quilkin proxy is identified as <IP_ADDRESS>.
In the first game, involving clients <IP_ADDRESS>:38400 and <IP_ADDRESS>:38402, communicating with the game server <IP_ADDRESS>:8884, everything is functioning correctly.
However, in the second game, involving clients <IP_ADDRESS>:38404 and <IP_ADDRESS>:38405, communicating with the game server <IP_ADDRESS>:7262, the client <IP_ADDRESS>:38404 did not receive any response.
An example request can be identified with correlation_id: 044bbeb (Started from the packet No. 1835).

Packet No. 1835: The client <IP_ADDRESS>:38404 sent a request to the proxy listener port at <IP_ADDRESS>:7777.
Packet No. 1836: The proxy forwarded the packet to the game server <IP_ADDRESS>:7262 from the socket at <IP_ADDRESS>:34118.
Packets No. 1837-1839: The game server sent three packets in response to the same socket <IP_ADDRESS>:34118.
Packets No. 1840-1842: The proxy forwarded these three packets to the other client at <IP_ADDRESS>:38405.

For visual reference:
We have a normal game:

And a problematic one:

To clarify the point a little further before I start going over unit tests and seeing if I can replicate this in one.

Is it intermittent where you get mis-routed packets, or once it starts, it doesn't stop?
Is there any chance the game server could be sending data to the wrong port in the proxy by accident?

To clarify the point a little further before I start going over unit tests and seeing if I can replicate this in one.

Is it intermittent where you get mis-routed packets, or once it starts, it doesn't stop?
Is there any chance the game server could be sending data to the wrong port in the proxy by accident?

It's intermittent, yes. Eventually, it occurs more frequently once it "starts."
From the pcap file, we can see that the socket Quilkin used to communicate with the game server <IP_ADDRESS>:7262 from two clients, <IP_ADDRESS>:38404 and <IP_ADDRESS>:38405, are both connecting to the same socket at <IP_ADDRESS>:34118. Although the game server sent responses to the same port, it is the client's port that the game server observed. Therefore, the answer is no (at least in this example).

Hmnn, not 100% sure I followed that. I need to double check the code, because I know this got optimised a while back (not by me, so I'm not as familiar anymore) so we could handle way more endpoints per proxy , but I'm fairly sure, it should be:

I.e. for each client connecting, there should be a different port the gameserver connects to to send packets back.
If it's the same port, I'm not sure how we differentiate which packet should go where 🤔 are you saying there is only one quilkin proxy port being used by the gameserver process?

If it's one port, I'm going to reckon that the load balancer is not preserving the IP:port of the client when sending traffic to the proxy, so to the proxy it looks like a single client.

@markmandel @XAMPPRocky Yes, we understand that the issue lies in the fact that the proxy uses the same port for two clients communicating with the game server. However, it is a symptom and not something we intentionally did. What we can confirm is:

We are running multiple proxies, but the problem arises when one specific proxy is used to route the traffic for those two clients.
The proxy listens to the two clients from two different source ports (same IPs) after passing through the load balancer.
The proxy forwards the packets from the clients to the game server using the same port.

(The No.2 and 3 can be observed in the dumped UDP traffic.

Yes, we understand that the issue lies in the fact that the proxy uses the same port for two clients communicating with the game server.

That wasn't what I was getting at. I was getting at the traffic from the proxy to the game server should be over 2 different ports at <IP_ADDRESS> as there should be a separate port for each backing client (and since there are 2, there should be two ports).
Is that what you are seeing?

The proxy forwards the packets from the clients to the game server using the same port.

 Without a different port on the proxy for each connection to the game server and back -- there's no way to differentiate where the traffic should head back to.

@zezhehh to clarify I mean that I'm not sure that the load balancer is always providing a unique ip:port pair. Not that you've made a change but however the load balancer works / is configured it is not always sending unique addresses.
Would you be able to test this with a NodePort for proxy traffic instead? I think cutting out the load balancer will help us determine if you can replicate it with direct traffic to the proxy.

Hmm.. I don't think the Load Balancer is the issue here. We have .spec.externalTrafficPolicy set to Local, as per the official doc:

Local preserves the client source IP and avoids a second hop for LoadBalancer and NodePort type Services, but risks potentially imbalanced traffic spreading.

@markmandel
Sorry for any confusion. I'll try to make it clear.

The proxy forwards the packets from the clients to the game server using the same port.

Yes, it's what we're seeing. The game server records the client socket (which is actually the proxy socket) for each client, identified by user ID, and responds to this specific socket. In other words, the game server doesn’t check for conflicts with other sockets but simply sends the response to the originating socket.
This situation only occurs when the error arises. In most cases, everything functions as expected: two clients from two sockets...

Got it - thanks.
Also, I assume there more than one endpoints in play at this point as well? (just to replicate as close as we can in a unit test to see if we can replicate).

Also, I assume there more than one endpoints in play at this point as well? (just to replicate as close as we can in a unit test to see if we can replicate).

@markmandel Yes, those automated testing matches occur within the same cluster as the real matches.

I finally got some time to look into this - check out the test I wrote in #1010 -- unfortunately I could not replicate any of your reported issues
Would love you to look at the test though, see if there is something else to the scenario that I didn't manage to capture in the integration test. Let me know if you see anything.

I finally got some time to look into this - check out the test I wrote in #1010 -- unfortunately I could not replicate any of your reported issues
Would love you to look at the test though, see if there is something else to the scenario that I didn't manage to capture in the integration test. Let me know if you see anything.

hmm.. Could you try to allocate the clients with the same ip and different ports?

hmm.. Could you try to allocate the clients with the same ip and different ports?

Unless you mean something else, the unit test has two sockets on the same IP (localhost) but different ports -- so I believe this tests this scenario, unless I am misunderstanding?

hmm.. Could you try to allocate the clients with the same ip and different ports?

Unless you mean something else, the unit test has two sockets on the same IP (localhost) but different ports -- so I believe this tests this scenario, unless I am misunderstanding?

Okay then all good. Thanks! We have some other different setup (same tokens from clients, etc.), but let's talk tomorrow! :)

Just for easy discovery, assuming there's an issue in Quillkin, it's likely one of these spots:

https://github.com/googleforgames/quilkin/blob/main/src/components/proxy/sessions.rs
https://github.com/googleforgames/quilkin/blob/main/src/collections/ttl.rs

So weird.


news endpoint or conid in client is out of date

Hi,
Thank you for your work and sharing. I had a test script working yesterday but not today. It seemed the hard coded news endpoint was out of date so I replaced it with an account endpoint ...
line 202 - 204 - commented out
line 342 ...
try:
# news_resp = self.data_news(conid='265598')
accs = self.portfolio_accounts()
if len(accs) > 0:
self.authenticated = True
                    #  Log the response.
                    logging.debug('Had to do Account Update Response: {auth_resp}'.format(
                            auth_resp=accs
                        )
                    )
                    break

It solved the authentication problem for me! Many thanks!

@areed1192 Did you access these endpoints with an older version of the clientportal? I can see in your resources folder that you have responses for this endpoint. Were those example responses generated using the Restful API, or accessing the url while logged into the web portal?


The attached spreadsheets got "outdated" - the list no longer reflect full list of processes.

The attached spreadsheets got "outdated" - the list no longer reflect full list of processes.
*https://view.officeapps.live.com/op/view.aspx?src=https%3A%2F%2Fdownload.microsoft.com%2Fdownload%2F6%2Fb%2Ff%2F6bfff670-47c3-4e45-b01b-64a2610eaefa%2Fmde-urls-commercial.xlsx&wdOrigin=BROWSELINK
*https://view.officeapps.live.com/op/view.aspx?src=https%3A%2F%2Fdownload.microsoft.com%2Fdownload%2F6%2Fa%2F0%2F6a041da5-c43b-4f17-8167-79dfdc10507f%2Fmde-urls-gov.xlsx&wdOrigin=BROWSELINK
The https://view.officeapps.live.com/op/view.aspx?src=https%3A%2F%2Fdownload.microsoft.com%2Fdownload%2Fe%2Fc%2F9%2Fec9e81cd-b39a-4d4c-afac-ff64cfe872ad%2Fmde-streamlined-urls-commercial.xlsx&wdOrigin=BROWSELINK already have updated list of processes (SenseTVM.exe included).

Document Details
⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

ID: 5046332b-8ef2-2023-1dc9-735cf33686c9
Version Independent ID: 5046332b-8ef2-2023-1dc9-735cf33686c9
Content: Configure your network environment to ensure connectivity with Defender for Endpoint service
Content Source: microsoft-365/security/defender-endpoint/configure-environment.md
Service: microsoft-365-security
Sub-service: mde
GitHub Login: @mjcaparas
Microsoft Alias: macapara

To make it easier for you to submit feedback on articles on learn.microsoft.com, we're transitioning our feedback system from GitHub Issues to a new experience.
As part of the transition, this GitHub Issue will be moved to a private repository. We're moving Issues to another repository so we can continue working on Issues that were open at the time of the transition. When this Issue is moved, you'll no longer be able to access it.
If you want to provide additional information before this Issue is moved, please update this Issue before December 15th, 2023.
With the new experience, you no longer need to sign in to GitHub to enter and submit your feedback. Instead, you can choose directly on each article's page whether the article was helpful. Then you can then choose one or more reasons for your feedback and optionally provide additional context before you select Submit.
Here's what the new experience looks like.
Note: The new experience is being rolled out across learn.microsoft.com in phases. If you don't see the new experience on an article, please check back later.
First, select whether the article was helpful:

Then, choose at least one reason for your feedback and optionally provide additional details about your feedback:

Article was helpful
Article was unhelpful

Finally, select Submit and you're done!

@mjcaparas Please help us in resolving this issue. Thanks


adds code coverage report

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@c9173b5). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #27   +/-   ##
=======================================
  Coverage        ?   96.95%
=======================================
  Files           ?       16
  Lines           ?      296
  Branches        ?       22
=======================================
  Hits            ?      287
  Misses          ?        8
  Partials        ?        1

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 c9173b5...bdf57e8. Read the comment docs.


ShellPage Workaround for NavigationView SelectionChanged

Summary of the Pull Request
ShellPage Workaround for NavigationView SelectionChenged

PR Checklist

[X] Closes: #21576
[ ] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
[ ] Tests: Added/updated and all pass
[ ] Localization: All end user facing strings can be localized
[ ] Dev docs: Added/updated
[ ] New binaries: Added on the required places

[ ] JSON for signing for new binaries
[ ] WXS for installer for new binaries and localization folder
[ ] YML for CI pipeline for new test projects
[ ] YML for signed pipeline

[ ] Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments
NavigationView in narrator mode (scan activated also) does not work properly. The selected item gets only selected, the navigation does not occure. Added code to force the navigation (loading of the corresponding page)

Validation Steps Performed
tested locally

Have we filed a bug against winappsdk?
@gabbybilka as FYI

A bug issue was filled here: https://github.com/microsoft/microsoft-ui-xaml/issues/8001

We've checked the WinUI 3 gallery, and a similar workaround is used for this component: https://github.com/microsoft/WinUI-Gallery/blob/353c1a0dab0aec9485179ad2a8b76cab95b7c8b9/WinUIGallery/ControlPages/NavigationViewPage.xaml.cs#L60-L199


Ecp

Token Address
https://scan.tomochain.com/tokens/0x5650ea7e65057b68d59fb5a62be25adf94c805bf
Token Symbol
ECP
Token Name
Ecommerce platform
Token Logo
https://drive.google.com/file/d/1g1_pYa_0rWZW6SOBjTI1S-QzEnlxrclL/view?usp=sharing
Token Description
ECP is a TRC21 token used as a means of payment at https://ecp.exchange
https://ecp.exchange is an e-commerce platform where buyers and sellers exchange directly with each other without intermediaries
Website
https://ecp.exchange
Social links
https://github.com/EcpExchange,https://t.me/ecpexchange,https://twitter.com/EcpExchange,https://www.facebook.com/groups/ecptokentrc21

Please approve Github
Thank you so much

Please approve Github soon
Thank you so much

When is it approved?

Please approve Github
Thank you so much

When is it approved?

Please approve Github

We change the rule because so many spam token request update info. Please read more in here https://github.com/tomochain/tokens

Do I need to contribute 20 or 30 tomo to this smart contract address correct?

Do I need to contribute 20 or 30 tomo to this smart contract address
correct?
Vào Th 7, 21 thg 8, 2021 vào lúc 09:22 Do Minh Khai <
@.***> đã viết:

We change the rule because so many spam token request update info. Please
read more in here https://github.com/tomochain/tokens
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/tomochain/tokens/issues/330#issuecomment-903039519,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AU7VT3GG6VGIEYZBRX64LTLT54EWLANCNFSM5B2CGLCQ
.
Triage notifications on the go with GitHub Mobile for iOS
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
or Android
https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email
.

I sent 20 TOMO
Please approve the update for ECP: https://scan.tomochain.com/tokens/0x5650ea7e65057b68d59fb5a62be25adf94c805bf
sent 20 Tomo
https://scan.tomochain.com/txs/0xdb6ca8d221a25e19665993b7e713b649705f80e0117e47c7f693ba0d578d3d3c

I sent 20 TOMO
Please approve the update for ECP:
https://scan.tomochain.com/tokens/0x5650ea7e65057b68d59fb5a62be25adf94c805bf
sent 20 Tomo
https://scan.tomochain.com/txs/0xdb6ca8d221a25e19665993b7e713b649705f80e0117e47c7f693ba0d578d3d3c
Thank you so much
Vào Th 2, 23 thg 8, 2021 vào lúc 14:53 Exchange ECP <
@.***> đã viết:

Do I need to contribute 20 or 30 tomo to this smart contract address
correct?
Vào Th 7, 21 thg 8, 2021 vào lúc 09:22 Do Minh Khai <
@.***> đã viết:

We change the rule because so many spam token request update info. Please
read more in here https://github.com/tomochain/tokens
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/tomochain/tokens/issues/330#issuecomment-903039519,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AU7VT3GG6VGIEYZBRX64LTLT54EWLANCNFSM5B2CGLCQ
.
Triage notifications on the go with GitHub Mobile for iOS
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
or Android
https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email
.

merge


Forgot Password & Sign Up option missing on Login page

Forgot Password option is not there on the login page
Sign Up or Register Option is also missing on login page in case it is a new user who wishes to sign up
The login page can be made more attractive through appropriate styling like by adding mail and lock icons with email and password
Moreover fonts can be made more appealing.

@sunny0625 @khushi-joshi-05
Hey, @khushi-joshi-05. I'm a beginner looking to contribute to your project. I noticed an opportunity to improve the login page:
these are few things that i can contribute:
Clear Labels: Ensuring the links are clearly labeled as "Forgot Password" and "Sign Up" to avoid any confusion for users.
Consistent Styling: Maintaining consistent styling with the rest of the login page to make these links visually identifiable but not distracting.
Mobile Optimization:  the links can be easily accessible and usable on mobile devices by placing them in a prominent location and optimizing for touch interaction.
please assign this issue to me under gssoc24

@khushi-joshi-05 @sunny0625
I would love to work on this project and make some contributions from my end.
Thanks


Change day formatter to be zero-padded

As discussed, changed the day part of the date formatter to be zero-padded (%d) instead of space-padded (%e).

Ah! Thanks for doing this @emredjan :)
I'll release a new version later today.

No, thank you for the amazing work on this.
A bit off-topic: I don't know if it was among your use-cases when you built, but this has been my go-to tool of choice for teaching SQL in my organization. Combined with a JupyterHub installation, it enabled me to provide users with an on-premise environment where they can access internal data using SQL without installing anything. Also the fact that we can just use pure SQL with proper syntax highlighting without resorting to any Python trickery is great for keeping users focused.
So yeah 🙌🏼

I'd say it warms my heart to read that, but it's too hot so I'll say it refreshes it hehe
Thanks for the kind words :)
Happy it's useful for you!

Yeah, no heart-warming during the heatwave, sorry

I'll release a new version later today.

😢

Oh! Haha
I'll do it now :)
Sry for that


Handle login errors without redirect

Depends on #10
If the login fails, display an error message in the UI without redirecting to the vld_validacao.validacao.

When the user has the login credentials stored and the user is not authenticated, while the page doesn't reload to insert the new session, the form should disappear and show an circular progress indicator instead (this is maybe only visible for 0.5 secs on good internet, but on slower ones it might be significant to avoid the same input from the user).


Cd new artifact sources cds 69354

Harness Developer Pull Request
Thanks for helping us make the Developer Hub better. The PR will be looked at
by the maintainers.
What Type of PR is This?

[ ] Issue
[* ] Feature
[ ] Maintenance/Chore

If tied to an Issue, list the Issue(s) here:

CDS-69354 CD doc :: add AWS S3, AMI, Google Cloud Storage to artifact sources topic

House Keeping
Some items to keep track of. Screen shots of changes are optional but would help the maintainers review quicker.

[ *] Tested Locally
[ ] Optional Screen Shoot.

Please check the Execution Link of the Pipeline for the Website Draft URL. This is located in the Preview Step and also is available in #hdh_alerts. E.g Website Draft URL: https://unique-id--harness-developer.netlify.app

Please check the Execution Link of the Pipeline for the Website Draft URL. This is located in the Preview Step and also is available in #hdh_alerts. E.g Website Draft URL: https://unique-id--harness-developer.netlify.app

Please check the Execution Link of the Pipeline for the Website Draft URL. This is located in the Preview Step and also is available in #hdh_alerts. E.g Website Draft URL: https://unique-id--harness-developer.netlify.app

Please check the Execution Link of the Pipeline for the Website Draft URL. This is located in the Preview Step and also is available in #hdh_alerts. E.g Website Draft URL: https://unique-id--harness-developer.netlify.app


setup.py: Block urllib3 >=2 for PyPy (CI kept failing; alternative to #786)

In reaction to https://github.com/kevin1024/vcrpy/pull/775#issuecomment-1847849962
Alternative to #786

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

Comparison is base (c062c9f) 90.10% compared to head (0e57182) 89.52%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
- Coverage   90.10%   89.52%   -0.59%
==========================================
  Files          27       27
  Lines        1809     1804       -5
  Branches      335      268      -67
==========================================
- Hits         1630     1615      -15
- Misses        134      144      +10
  Partials       45       45

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

@josumoreno-BP this is not the right place for a bug report and after re-checking the change in here, it still seems correct. I'm not sure I understand the poetry part. if this matter it will need a dedicated bug report and more elaboration, at least with me. Thanks.


Correct Paket package name to install for Saturn

Hey I just wanna fix a typo in the docs :)

Pull request checklist
Please check if your PR fulfills the following requirements:

[ ] Tests for the changes have been added (for bug fixes / features)
[ ] Docs have been reviewed and added / updated if needed (for bug fixes / features)
[ ] Build (fake build or .\build.cmd) on local branch was successful

Pull request type

Please check the type of change your PR introduces:

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, renaming)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] Documentation content changes
[ ] Other (please describe):

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

[ ] Yes
[ ] No

Other information

Oh whoops, thanks!


Arrangement naming convention for sumeragi and connection

This issue is about migrating-flatbuf branch, which is being hard restructured.
I think some names of methods and namespaces are obscure, therefore reading/writing codes are not easy. I'd like to propose some changes. If there are my misunderstandings, feel free to point out.

[ ] namespaces are obscure.

e.g.

::connection::iroha::SumeragiImpl
::sumeragi
::iroha::[FlatbuffersSchema]

Identifier iroha should be top most level namespace of program.
Namespace named as proper noun (e.g. sumeragi, izanami, ...) may be better at one place.

Giving namespaces proper noun is expected that they are unique. e.g. It's bad that there are connection::iroha and iroha::[FlatbuffersSchema].

Make names readable such as B in A if A::B.

::connection::iroha::SumeragiImpl needs to be read such as in case that iroha's sumeragi wants to use connection.

Follow ordinary naming convention. [Something]Impl is implementation of [Something] itself.
namespace should be noun. (namespace Verify is bad)

[ ] method names' are obscure.

SumeragiConnectionClient::Verify()

verify recalls just confirming data integrity, for example, validating data format, password, etc. SumeragiConnectionClient::Verify() calls other peer's Verify() method, which is not simply integrity checking.

connection::iroha::SumeragiImpl::Torii::receive()

receive might recall getting transaction from other peer or client, but this method set callback function that receives transaction.

For verify: there is stateless validation and stateful validation, so that distinction should be made.


Update prom/common to v0.43.0

Not sure what the upgrade protocol is. If its already planned, feel free to close this PR.

See #12412 but there is CI error.


Telemetry item gets lost when property value ends with a backslash

The exact same behavior as in issue #261 has recently surfaced. Our project is missing logs since two weeks ago. Due to the fact, that both the newest Application Insights NuGet package (2.21.0) and (2.20.0) show this behavior, I suspect it's an azure backend bug.
The obivous quick fix, removing ending backslashes, has been implemented at our end. The community will still profit, when this bug is removed, I assume.

@AndreasMossner can you share any sample code that reproduces this?

You can basically use the code from issue #261. I just updated the telemetry configuration.
using Microsoft.ApplicationInsights;
using Microsoft.ApplicationInsights.Extensibility;

TelemetryConfiguration configuration = TelemetryConfiguration.CreateDefault();
configuration.TelemetryChannel.DeveloperMode = true;
configuration.ConnectionString = "**MyConnectionString**";
var telemetryClient = new TelemetryClient(configuration);

var validProps = new Dictionary<string, string>
{
    {"WorksFine", @"`Does_Not_End_With_Backslash\1234567890-=[];',./~!@#$%^&*()_+{}:|<>?"}
};

var failedProps = new Dictionary<string, string>
{
    {"DoesNotWork", @"Testing\"}
};

telemetryClient.TrackEvent("No Properties - This works");
telemetryClient.TrackEvent("Valid Properties - This works", validProps);
telemetryClient.TrackEvent("Failed Properties - Does not work", failedProps);

telemetryClient.Flush();

@TimothyMothra Have you got a chance to look at it yet?

@AndreasMossner The code sample you provided works for me. All three events show in the portal:

Hi @AndreasMossner,
We're getting into an area that I can't troubleshoot over github.
Please consider opening an Azure Support Request so that our support teams can help investigate this. https://github.com/microsoft/ApplicationInsights-dotnet#support
You can reference this github issue.
From your screenshot, I can see that the SDK correctly created the Telemetry Item, but that item wasn't ingested.
I would need to see more detailed logs to understand what's happening here.
--
One last thing you can try is our Ingestion test script: https://github.com/microsoft/ApplicationInsights-dotnet/blob/main/troubleshooting/Ingestion/PostTelemetry.ps1
You can modify line 47 with the extra backslashes:
https://github.com/microsoft/ApplicationInsights-dotnet/blob/35b1a303f0fa07b9d93f178920ec65d400f729c8/troubleshooting/Ingestion/PostTelemetry.ps1#L46-L48
When you run this script, you should provide your connection string like this: PostTelemetry.ps1 -ConnectionString "<YOUR CONNECTION STRING HERE>"
When I run this script this is the output:
	"properties": {
		"TestProperty": "TestValue\\"
	}

VERBOSE: POST with 514-byte payload
VERBOSE: received 49-byte response of content type application/json
StatusCode        : 200
StatusDescription : OK
Content           : {"itemsReceived":1,"itemsAccepted":1,"errors":[]}
RawContent        : HTTP/1.1 200 OK
                    x-ms-session-id: FE62A3D3-8089-4EBD-B100-E1199923FB26
                    Strict-Transport-Security: max-age=31536000
                    Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Name, Content-Type,…
Headers           : {[x-ms-session-id, System.String[]], [Strict-Transport-Security, System.String[]], [Access-Control-Allow-Headers, System.String[]],
                    [Access-Control-Allow-Origin, System.String[]]…}
Images            : {}
InputFields       : {}
Links             : {}
RawContentLength  : 49
RelationLink      : {}

If I have invalid json, I get a response from Ingestion like this:
	"properties": {
		"TestProperty": "TestValue\"
	}

VERBOSE: POST with 513-byte payload
VERBOSE: received 127-byte response of content type application/json
Invoke-WebRequest: C:\REPOS\AISDK\dotnet\troubleshooting\Ingestion\PostTelemetry.ps1:63:1
Line |
  63 |  Invoke-WebRequest -Uri $Url -Method POST -Body $AvailabilityData -Ver …
     |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | {"itemsReceived":1,"itemsAccepted":0,"errors":[{"index":0,"statusCode":400,"message":"Invalid JSON specified in the request"}]}

@AndreasMossner, If you can capture that error message from the Ingestion service that would help narrow down the issue.

Hi, I just got the chance to try it.
With the standard json, I have that as an output:
VERBOSE: POST with 512-byte payload
VERBOSE: received -byte response of content type application/json

StatusCode        : 200
StatusDescription : OK
Content           : {"itemsReceived":1,"itemsAccepted":1,"errors":[]}
RawContent        : HTTP/1.1 200 OK
                    Transfer-Encoding: chunked
                    Server: Microsoft-HTTPAPI/2.0
                    X-Content-Type-Options: nosniff
                    Strict-Transport-Security: max-age=31536000
                    X-Request-ID: c554b285-b1fb-4c2d-a0aa-e4b57967…
Headers           : {[Transfer-Encoding, System.String[]], [Server, System.String[]], [X-Content-Type-Options,
                    System.String[]], [Strict-Transport-Security, System.String[]]…}
Images            : {}
InputFields       : {}
Links             : {}
RawContentLength  : 49
RelationLink      : {}

With testvalue\\ or testvalue\, I get:
VERBOSE: POST with 514-byte payload
VERBOSE: received -byte response of content type application/json
Invoke-WebRequest: C:\Repos\TriggerAppInsights\TriggerAppInsight2\IngestAppIns.ps1:63
Line |
  63 |  Invoke-WebRequest -Uri $Url -Method POST -Body $AvailabilityData -Ver …
     |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | {"itemsReceived":1,"itemsAccepted":0,"errors":[{"index":0,"statusCode":400,"message":"Invalid JSON specified in
     | the request"}]}

@AndreasMossner, this sounds like a bug in the Ingestion Service.
Please open an Azure Support request so their team can investigate this: https://github.com/microsoft/ApplicationInsights-dotnet#support
You can reference this github issue.
Can you please share here the IngestionEndpoint in your connection string?
Example: IngestionEndpoint=https://westus2-0.in.applicationinsights.azure.com/

The Ingestione Endpoint:
IngestionEndpoint=https://westeurope-.in.applicationinsights.azure.com/
I tried to open a support request, but since I apparantly only have a basic plan, it doesn't let me do that.
I don't really get why that's the case, though. Neither does the problem seem to be on my end, nor do I need technical support. I just wanted to report a bug. Sadly, other people also seem to have the same problem.
I also don't get why I only have a basic support plan. Although my company has other subscriptions for productive systems, besides the personal visual studio abonnements, I apparantly can't open tickets on one of those.

Issue acknowledged. Looking into it.

@AndreasMossner, thank you for reporting the issue! We have a fix ready. Since it touches the most exercised path it will take time for us to validate it and roll it out. It should reach West Europe by early Jan (might reach earlier but not guaranteed).
Again, thank you for reporting the issue and providing very clear explanation and simple repro!


Adding the embedders to bio_embeddings

Hey folks :)
Great work!. As I mentioned on Twitter, it'd be nice to add your models to bio_embeddings. Purpose of the pipeline: make it easy for less-tech-savy bio/informatician to use protein LMs. Since you use torch, this should be quite straightforward since we already have some transformer models.
Out of the box, you get the whole "read FASTA in, make run reproducible", project, viz & embedding annotation transfer (goPredSim) pipelines. Edit: oh, and the auto-batching of large sequence files between GPU/CPU (which is not at all intuitive for the avg user), + per-sequence vs per-AA representations (looking through closed issues, #2 )
I noticed you have some variant prediction code, maybe it makes sense to include that as a pipeline step if it is sensible?
I'll link this to our issue for integration so that we can cross-follow the status: https://github.com/sacdallago/bio_embeddings/issues/62

Thanks Chrstian, We'll be happy to work with you folks to make this happen! The team is a bit swamped right now, but will look into this soon!

I've added ESM to bio_embeddings: https://github.com/sacdallago/bio_embeddings/blob/0b7d5f32ec7743b5fbdfcbede5bf2f0e1cbdb52b/bio_embeddings/embed/esm_embedder.py. It would be great if you could check whether the implementation looks correct to you.
Could publish ESM on pypi? I've seen that you have a proper python package (thanks for that!) and could verify with testpypi that python setup.py sdist bdist_wheel && pip instal twine && twine upload dist/* works.

Also: @tomsercu is your blog's gmail address still the best one to reach you? :) I sent you something ~2 days ago, just wondering :)

I think this is pretty much done, except we'll now also add the new model :P Congrats on that one, too.


Creating type definitions for v1.2

@Roundaround Please push an empty commit to trigger the status checks:
$ git commit --allow-empty -m "Trigger status checks"
$ git push

This has been happening a lot lately 😞

The pipeline is down; I will look into it tomorrow.

Pipeline fixed (#125); 1.2.14-pre.0 now available.


feat(ci): Add e2e test bed

[x] Launch control plane via docker compose
[x] Ensure all clients build

So this just builds a debug package, and the next big change would be to install the MSI and smoke-test it?

Correct. Though we may not even need to do that. See https://tauri.app/v1/guides/testing/webdriver/introduction


Changing opened win.document Title from 'SheetJS Table Export'

Hello,
I'm currently embedding a table from an Excel sheet into a document & opening it as a new tab in Chrome.
let worksheet = workbook.Sheets[workbook.SheetNames[0]];
    var win = window.open("", "", "toolbar=no,location=no,directories=no,status=no,menubar=no,scrollbars=yes,resizable=yes,fullscreen=yes,width=" + screen.width + ",height=" + screen.height);
    win.moveTo(0, 0);
    win.document.body.innerHTML = XLSX.utils.sheet_to_html(worksheet);

However, since the document's  tag has no id, I'm having difficult changing it something other than 'SheetJS Table Export.'

Is there any way to change the document's title?

You can override the header:
XLSX.utils.sheet_to_html(worksheet, {
  header: '<html><head><meta charset="utf-8"/><title>' + title + '</title></head><body>'
});

If you just want the table without any preamble or postamble:
XLSX.utils.sheet_to_html(worksheet, { header: "", footer: "" });


Merging to main without bump fails CI.

For example:
Run cz bump --changelog --yes
bump: release 0.10.5 → 0.10.5

Automatically generated by Commitizen.

tag to create: v0.10.5
[NO_COMMITS_TO_BUMP]
The commits found are not eligible to be bumped

Error: Process completed with exit code 21

We probably shouldn’t fail CI here. However, looks like cz doesn’t support this on bump 🤔

At @Lee-W’s suggestion:
cz --no-raise 21 bump --changelog --yes

PR https://github.com/jenstroeger/python-package-template/pull/428, commit https://github.com/jenstroeger/python-package-template/commit/8f9b1934e4191dc6060bc5bfab2d2985494faeb5.


JPEG decoding and encoding is significant slower than from Ubuntu package

First of all thanks for your work!
OpenCV version in Ubuntu 16.04 is still <IP_ADDRESS>, so most users have to install OpenCV from this wheels.
Surprisingly, these wheels have much lower JPEG decoding and encoding speed than shipped with Ubuntu.
Cv2 load 2560×1600 RGB image
    Jpeg load           0.05430 s    75.43 Mpx/s
    Jpeg save           0.10867 s    37.69 Mpx/s
Cv2 load 2560×1600 RGB image
    Jpeg load           0.03277 s   124.99 Mpx/s
    Jpeg save           0.03084 s   132.82 Mpx/s

Here is perf report for both cases:

It's clear that these wheels don't use system-provided libjpeg library (which is libjpeg-turbo for some systems). Instead, they are decoding and encoding using some build-in libjpeg library which is much slower.
I see two solutions: use libjpeg-turbo for wheels or use system libraries.

I've tried to build recent version of  OpenCV from sources, it also uses system codecs and run fast

Yes, due to the nature of the wheel packaging format requirements (restricted subset of the Linux kernel and core userspace ABI) almost all of the needed libraries are embedded into the wheels. However, libjpeg's case is a bit different because the it's most likely included in the static OpenCV binary.
I'm pretty sure I can change the library to libjpeg-turbo, but it requires a lot of manual tinkering on the CentOS 5 Docker containers (like all the other open Linux related issues which are currently open).

Next release will ship with libjpeg-turbo 1.5.3.


FMB.c

The Flux Memory Balancer (FMB) algorithm dynamically allocates memory to processes based on their priority and memory requirements, ensuring efficient utilization of available memory. Higher-priority processes receive their requested memory first, while lower-priority processes may receive reduced memory if total demand exceeds capacity. If necessary, FMB redistributes memory by incrementally reducing allocations from lower-priority processes.

@pankaj-bind Please Review my PR.


[DDW-683] Refactor the Loading screen

This PR refactors the Loading screen, adding containers for each overlay and splitting it into smaller components.
Todos

[x] Reorganize the files, so the Loading container loads the overlays containers
[x] Break the Loading component into smaller components
[x] Update storybook stories

Screenshots
SyncingConnecting

NoDiskSpaceError

SystemTimeError

ManualUpdate

Testing Checklist

Slack QA thread
[ ] SyncingConnecting. This screen has a lot of props combinations, so it's better to test in the actual app.
[ ] NoDiskSpaceError
[ ] SystemTimeError
[ ] ManualUpdate

Review Checklist
Basics

[ ] PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
[ ] PR is updated to the most recent version of the target branch (and there are no conflicts)
[ ] PR has a good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
[ ] CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
[ ] Automated tests: All acceptance and unit tests are passing (yarn test)
[ ] Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
[ ] Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
[ ] There are no flow errors or warnings (yarn flow:test)
[ ] There are no lint errors or warnings (yarn lint)
[ ] There are no prettier errors or warnings (yarn prettier:check)
[ ] There are no missing translations (running yarn manage:translations produces no changes)
[ ] Text changes are proofread and approved (Jane Wild / Amy Reeve)
[ ] Japanese text changes are proofread and approved (Junko Oda)
[ ] UI changes look good in all themes (Alexander Rukin)
[ ] Storybook works and no stories are broken (yarn storybook)
[ ] In case of dependency changes yarn.lock file is updated

Code Quality

[ ] Important parts of the code are properly commented and documented
[ ] Code is properly typed with flow
[ ] React components are split-up enough to avoid unnecessary re-renderings
[ ] Any code that only works in main process is neatly separated from components

Testing

[ ] New feature/change is covered by acceptance tests
[ ] New feature/change is manually tested and approved by QA team
[ ] All existing acceptance tests are still up-to-date
[ ] New feature/change is covered by Daedalus Testing scenario
[ ] All existing Daedalus Testing scenarios are still up-to-date

After Review

[ ] Merge the PR
[ ] Delete the source branch
[ ] Move the ticket to done column on the YouTrack board
[ ] Update Slack QA thread by marking it with a green checkmark

@daniloprates Looks good.

@a-rukin please ping me once you start today so that I can show you what I mean...

@tomothespian please review the code too 🙏

The main point of the transparent bg for all these overlays is to show that something you have been working with is still there and waiting for you.
We (will) have only one overlay with no transparency - locking screen where we want to hide all sensitive data, so it's not an overlay actually, but a completely separate screen without anything underneath.


Feature request: Stretchable blocks for buttons

Is your feature request related to a problem? Please describe.
Although I translate the application into Russian, I personally use Ukrainian everywhere. And since changes weren't always made to the Russian translation in a timely manner, and I possess Russian at a sufficient level to be able to translate, I decided to support this translation as well, because I simply can. There are probably many people who use the application and the Russian language in it, so it's more for them. But less lyrics.
I was passing the file to an acquaintance who uses Russian and noticed that the inscription "Приложение" (App), doesn't fit into the block for button, since it's long, so the font for it's reduced (if you remember, this was the case with the Clipboard earlier). And it doesn't look pretty at all. And the word "Приложение" (App), for such a case there is no correspondingly correct synonym to replace it and there is also no abbreviation, such as App (Application) in English. The only option is the word "Программа" (Program), but it's a bad match in this case. And this word is only one letter shorter, so it's not guaranteed that it'll be suitable either.
I don't know if it's possible to implement such an option, but according to my logic it should be possible. As far as I understand, currently, if some text doesn't fit in the block for button, the font size is reduced to fit it. Perhaps it would be possible to implement so that the font size doesn't decrease, but just so the block for button itself was stretched in width until the inscription would fit in it. For example, there is a minimum size for a block for button as it's now. And if all inscriptions fit into the block for button, then accordingly, nothing happens. But if the inscription doesn't fit into the block for button, then the block increases in width, for example, by 5% (because mostly only 1-2 letters don't fit), if it doesn't fit further, then by another 5%, and so on. Of course, there should be some limiting parameter so that the block for one button doesn't take up the entire screen. For example, the maximum allowable button size is no more than 50% of the original size. And if such an increase occurs for a button/s, then the size for all buttons in this case is set the same – the size of the largest button. That is, if the size of one button has increased by 105%, and another, for example, by 110%, then the size of all buttons in this case will be 110%. Even for those buttons that didn't stretch at all. This is simply to preserve the existing symmetry for all the buttons.
The same option can be applied to the settings tab, but there, the blocks would stretch in height (although in the case of settings, it's not as critical and noticeable). Of course, this is a lot of IF operators in theory, but it'll not break the design of the application at all and will allow you to use more correct inscription, instead of looking for synonyms which, in many cases, are not entirely suitable. Of course, this won't allow you to enter 100% of all the desired inscriptions, because in some cases they can really be too long and really need to be shortened, but both in Ukrainian and in Russian languages there are a lot of long words, and when literally 1-2 letters don't fit, and the font size is reduced, it's visually unpleasant. And in some cases, as, for example, in this case, the word cannot be replaced by anything. Therefore, we get such a stalemate, where the options are either to come up with an abbreviation for the word yourself, or to put up with a lowercase font for one button.
Describe the solution you'd like
Described above.
Describe alternatives you've considered
If the solution I proposed isn't feasible, too cumbersome, or simply not to your liking, then in that case, I'd be very grateful if you could at least stretch the blocks for buttons in width slightly so that they could accommodate 10 characters, there are exactly 10 letters in the word "Приложение" (App). It's literally a few pixels. Because I can't find any adequate substitute for this word.
Additional context

Thanks for this notice! I have removed the maximum width constraint of each button. I think this should be fine for now.
Adjusting the width of all buttons would be too difficult.
The Russian "Приложение" name is really long so it would make the other buttons that long that the user has to scroll a while to reach the last button.

Thanks for this notice! I have removed the maximum width constraint of each button. I think this should be fine for now.

Yes, such behavior is much better.

Adjusting the width of all buttons would be too difficult.

Clearly. I suspected as much. But removing the width restriction for each button isn't bad either. At least, it visually looks better than reducing the font size.

The Russian "Приложение" name is really long so it would make the other buttons that long that the user has to scroll a while to reach the last button.

Well, in the Ukrainian word "Додаток", there are only three letters less than in the similar Russian "Приложение". Therefore, the size wouldn't be radically different from the current one. However, I wouldn't want it to affect all users, so the option of removing the width restriction for each button is a great compromise.

@Tienisto Regarding "the user has to scroll a while to reach the last button": I don't want to open a separate issue for this, so I'll mention it here. If you wish, you can format this inquiry as a separate issue. I just don't know if you'll consider it at all, so I'll leave it here.
Currently, many users aren't even aware that besides the four visible buttons, there is an additional one, and this list can be scrolled. Well, at least, all those with a standard screen and aspect ratio, and the fifth button or its part (narrow screen on Sony) is simply not noticeable to them. I deliberately showed friends that on Android, you can share apps, which is the fifth additional button. Although the indents on the left and right are different, not everyone can guess that this is due to additional elements.
If you pay attention, in most such cases, they show an additional part of the next element for this purpose. A vivid example of this is the display of workspaces in GNOME, which came to mind first. There it's implemented in this way (screenshot 1). I don't know how difficult it'll be to implement such an option here, but you can implement it, in my opinion, in a slightly simpler way (screenshot 2), although perhaps it's not as obvious and common. Specifically, by adding a "scroll" button next to the "Selection" section. And when pressed, the entire list of section buttons is automatically scrolled to the last one.
You can also add animation where the button rotates in the opposite direction so that when the user presses it again, the list automatically returns to the first section button. A reverse action. In my opinion, this would look nice.
P.S. I'll also note that when I created the image with scrolling, I noticed that not all icons are aligned in color. Some of them are lighter, and some are darker, for example, the "Settings gear icon" and "the heart icon for Favorites". It would also be good to fix this.

I'm also attaching the icon just in case.
scroll.zip

@Tienisto Sorry to bother you again, but the thing is that not all of my Russian-speaking acquaintances liked the fact that now one button is wider than all the others. It turned out that they preferred the previous option where the font on the button simply decreased and adjusted to fit the button, and all the buttons were of the same size. Indeed, most of them liked the current version with a wider button. But, roughly speaking, opinions on this matter are divided.
So, I've a request, if possible, please add a toggle switch in Advanced settings through which one could enable the maximum width limitation for each button, as it was before.

I don't think it is something that should be configurable because it adds unecessary complexity to the app.
I am indifferent regarding the "stretchable" buttons though.
Another solution would be to display 2 rows for languages where it doesn't fit in one line.

I don't think it is something that should be configurable because it adds unecessary complexity to the app. I am indifferent regarding the "stretchable" buttons though.

I'm completely satisfied with the wider button. In my opinion, it looks visually better than reducing the font size. I was asked to convey this to you, and I've done so. I'll communicate your response to them.
Sorry again for any inconvenience.

Another solution would be to display 2 rows for languages where it doesn't fit in one line.

As for displaying the word on two lines, there might be issues with the word being grammatically divided incorrectly. The current option, in my opinion, is the most optimal among all possibilities.

@Tienisto
Sorry to bother you again about this matter, but I wanted to inquire whether the issue of reducing internal indents to make the text on the button occupy more useful space has been resolved? Are there certain restrictions on this and everything will remain as it's now? A short answer will be enough so as not to waste your time.
The reason I ask is that we've managed to find an alternative for the word "Приложение" (Application) which seems to suit everyone, more or less. However, it's essentially just a physical abbreviation of the word "Приложение" to "Прилож.", and it doesn't look very good or nice to me, but as I mentioned earlier, it's much more suitable for people who directly use the Russian language than the current version with a really wide button. Regarding the length of the word "Прилож.", it's identical in terms of character count and size to the word "Додаток" (Application), which is used in the Ukrainian language, so it should fit well in terms of size without changing the width of the button itself. But I would sincerely appreciate it if you could test this in practice before releasing the next version.
Regarding changing the width of the button. To avoid prolonging the discussion, I'll write in advance if the issue regarding internal indents has NOT BEEN RESOLVED.
In summary. Through all the discussions, it turned out that the previous option, where the button size is static, actually appeals to most people with a really good argument, in my opinion. "Reducing the font size on the button encourages the translator to seek alternatives and changes, whereas increasing the width of the button is perceived as more normal and doesn't stimulate further search for alternatives, so everything will remain unchanged, even if the button turns out to be very wide". And the increase in the width of the button, as it turned out, is really quite significant due to large indents, and even the growth of the word by 1 character noticeably increases the width of the button itself in relation to others. And it's really very noticeable, and it really doesn't look very nice. Ultimately, as I was told, this isn't very good for people who simply use the app in their native language and don't participate in the translation process. Therefore, if you haven't received any more complaints regarding this issue, perhaps it's indeed worth reverting to the previous option with static button width size.

Thanks for your response @sergd88
I have tried to remove the padding, here is a screenshot of this change:

Thanks for your response @sergd88
I have tried to remove the padding, here is a screenshot of this change:

Oh, that's great. I'm truly appreciative ❤


Module 1.3 Assignment

Stage changes following first greeting

Commit

Changes following second greeting

Final commit

@JamesPEarly Thank you for separating the commands 😄


feat: cosmetic tweaks for Neovim

This is a draft for now because it would require bigger changes to directory layout in order to fully complete it. If you want me to make those changes, let me know and I can update.
Before this PR

After this PR

With a syntax file

That last screenshot shows what the non-breaking spaces are all about: they provide us with something we can conveniently syntax-highlight to make the headers stand out:
if vim.fn.exists('main_syntax') == 0 then
  if vim.fn.exists('b:current_syntax') == 1 then
    return
  end
  vim.g.main_syntax = 'shellbot'
elseif vim.fn.exists('b:current_syntax') == 1 and vim.b.current_syntax == 'shellbot' then
  return
end

vim.cmd('runtime! syntax/markdown.vim')

local cpo = vim.o.cpo

vim.cmd([[
  set cpo&vim
  syntax match ChatBotHeader /^ 🤓 .*/ containedin=ALL
  syntax match ChatBotHeader /^ 🤖 .*/ containedin=ALL
  highlight def link ChatBotHeader TermCursor
]])

vim.b.current_syntax = 'shellbot'
if vim.g.main_syntax == 'shellbot' then
  vim.api.nvim_del_var('main_syntax')
end
vim.o.cpo = cpo

So, why can't I include the syntax file in this PR? Because syntax files have to live under a syntax directory, but the repo doesn't have the right layout right now; ie. the current layout is:
<root>
├── Cargo.lock
├── Cargo.toml
├── LICENSE
├── README.md
├── chatbot.lua
├── shellbot
│   └── health.lua
├── shellbot.sh
└── src
    ├── anthropic.rs
    ├── api.rs
    ├── main.rs
    ├── openai.rs
    └── sse.rs

but the final layout would need to be something like:
<root>
├── Cargo.lock
├── Cargo.toml
├── LICENSE
├── README.md
└── lua
|   ├── chatbot.lua
|   └── shellbot
|       └── health.lua
└── syntax
|   └── shellbot.lua
├── shellbot.sh
└── src
    ├── anthropic.rs
    ├── api.rs
    ├── main.rs
    ├── openai.rs
    └── sse.rs

ie.

move Neovim lua files into lua subdirectory; this is where Neovim expects to find Lua files such that they can be imported with require('shellbot').
create syntax/shellbot.lua; standard location for syntax files, automatically loaded when shellbot 'filetype' is activated.

Draft for now for reasons indicated. Let me know what you think, @wolffiex.

This is awesome! Thanks for the contribution!

Superseded by:

https://github.com/wolffiex/shellbot/pull/12


add combine_to_tibble

Resolves #140 with this behavior:
> tfiles <- tempfile(pattern=as.character(1:3))
> file.create(tfiles)
[1] TRUE TRUE TRUE

> combine_to_tibble()
Error in length(...) : 0 arguments passed to 'length' which requires 1

> combine_to_tibble(tfiles[1])
# A tibble: 1 x 2
  filename                                                       hash
  <chr>                                                          <chr>
1 /var/folders/_0/fbg0ffkj6z3fb_7jvm8wg86r002c5h/T//RtmpjELF9G/… d41d8cd98f00b204e9800998e…

> combine_to_tibble(tfiles[1:3])
# A tibble: 3 x 2
  filename                                                       hash
  <chr>                                                          <chr>
1 /var/folders/_0/fbg0ffkj6z3fb_7jvm8wg86r002c5h/T//RtmpjELF9G/… d41d8cd98f00b204e9800998e…
2 /var/folders/_0/fbg0ffkj6z3fb_7jvm8wg86r002c5h/T//RtmpjELF9G/… d41d8cd98f00b204e9800998e…
3 /var/folders/_0/fbg0ffkj6z3fb_7jvm8wg86r002c5h/T//RtmpjELF9G/… d41d8cd98f00b204e9800998e…

Ugh, nothing is ever simple. Should this function support combining of objects? I guess I'm going to leave it out for now, because I got this far in rewriting it before realizing I couldn't rely on remake:::target_is_file or fetch without also allowing custom arguments in combiners (#116 ):
    tibble(name=c(...)) %>%
      mutate(hash = purrr::map(name, function(nm) {
        if(remake:::target_is_file(nm)) { # but what if it's a non-default file extension?
          unname(tools::md5sum(nm))
        } else {
          digest::digest(fetch(nm)) # and what if it's a non-default remake file?
        }
      }))

OK @jread-usgs , I added some tests and did a bit of cleanup for edge cases. See also comment above, but if the current changes seem like they can stand alone, let's just merge and be done with it for now.


ServiceProxy(): Forces JSON strings to be url decoded

Currently ServiceProxy() uses urllib, which in Python 2.x doesn't allow for HTTP header modification. This becomes an issue when trying to send JSON strings with characters that contain values that look like url-encoded characters. By default urllib sends POST data with the HTTP header "Content-Type" set to "application/x-www-form-urlencoded". The JSON client should be using "application/json" for the "Content-Type" in the HTTP header. The following code is meant to demonstrate the problem:
Server:
from flask import Flask
from flask_jsonrpc import JSONRPC
app = Flask("DEBUG")
annote = JSONRPC(app,'/DEBUG',enable_web_browsable_api=True)
@annote.method('one(str)->str')
def one(instr):
    print("string is %s" % instr)
    return instr

app.run(host='<IP_ADDRESS>',port=5001,debug=True)

Client:
from flask_jsonrpc.proxy import ServiceProxy
s=ServiceProxy('http://<IP_ADDRESS>:5001/DEBUG')
print(s.one('No%20Space'))

Resulting output:
{u'jsonrpc': u'2.0', u'id': u'df4932b0-8551-11e4-86b1-0026b9641702', u'result': u'No Space '}

The "%20" was decoded into a space (" ").
The solution for this would be to replace urllib with urllib2 in _compat.py and subsequently the import of urllib in ServiceProxy(). The default HTTP header "Content-Type" should be set to "application/json" but there will be a overriding parameter available so users can provide their own HTTP headers dictionary.
The following are code snippets that illustrate the solution:
_compat.py:
For PY3:
encode_filename = _identity
    get_next = lambda x: x.__next__

    from urllib.parse import urlparse
    from urllib.request import urlopen
    from urllib.request import Request

For PY2:
        return filename

    from urlparse import urlparse
    from urllib2 import urlopen
    from urllib2 import Request

proxy.py:
class ServiceProxy(object):

    DEFAULT_HEADERS = {"Content-Type":"application/json"}

    def __init__(self, service_url, service_name=None, version='2.0', headers=None):
        self.headers = headers or self.DEFAULT_HEADERS
        self.version = text_type(version)
...
...
...
    def send_payload(self, params):
        """Performs the actual sending action and returns the result
        """
        data = json.dumps({
            'jsonrpc': self.version,
            'method': self.service_name,
            'params': params,
            'id': text_type(uuid.uuid1())
        })
        data_binary = data.encode('utf-8')
        url_request = Request(self.service_url,data_binary,headers=self.headers)
        return urlopen(url_request).read()

Thanks for taking a look!

Hello,
I will integrate in new release.
Thank you.

Fixed in version 0.3.


[scratch][projects] Allow submitting PRs from scratches

Fairly untested right now and definitely needs more work, but just putting current progress out there.
Still needed:

[ ] Auth fixup: only request repo-write auth when needed
[ ] Better error handling (incl. when scratch is not a project fn)
[ ] Only let users submit PRs for matched funcs?
[ ] Disallow PRs if context has been modified?
[ ] UI: we should probably hide this functionality for now

When PRing was discussed previously, we had talked about having a decomp.me account to PR matches and co-author the author. Is this being shelved for now?

When PRing was discussed previously, we had talked about having a decomp.me account to PR matches and co-author the author. Is this being shelved for now?

@matt-kempster and I figured out how to use your github oauth token to make a fork of the project repo and create a commit as you, as if you had made it on github.com in the little web editor thingy. I think this is better and it doesn't necessitate a bot user

When PRing was discussed previously, we had talked about having a decomp.me account to PR matches and co-author the author. Is this being shelved for now?

@matt-kempster and I figured out how to use your github oauth token to make a fork of the project repo and create a commit as you, as if you had made it on github.com in the little web editor thingy. I think this is better and it doesn't necessitate a bot user

The fear we had when originally discussion was for needing full write permissions to decomp.me We figured certain users may have issues giving that significant of permissions to decomp.me. Does this oauth not have this issue?

It does, yes. The live version of the site actually already requests the public_repo scope (see #390) and nobody appears to have brought up that as an issue :shipit:
This feature will be opt-in though, i.e. when you first try and make a PR it'll send you to GitHub asking for repo write permissions

I'm personally fine with it, and I imagine many people will be as well. I just worry that there may be some users with legitimate code they cannot risk trusting those perms at all,

IMO its fine for decomp.me's position on users with qualms such as those to be "okay, make the PR yourself then"

IMO its fine for decomp.me's position on users with qualms such as those to be "okay, make the PR yourself then"

Yeah this or we eventually can introduce the bot user to do it for you

Disallow PRs if context has been modified?

I disagree on that point. Sometimes a function may indicate an issue within a helper function deeply buried within the context. If someone is able to find it, they should still be able to suggest (PR) that change. Maybe display a warning, so accidental changes don't happen too quickly, but it should still be possilbe at all.

Disallow PRs if context has been modified?

I disagree on that point. Sometimes a function may indicate an issue within a helper function deeply buried within the context. If someone is able to find it, they should still be able to suggest (PR) that change. Maybe display a warning, so accidental changes don't happen too quickly, but it should still be possilbe at all.

We want to allow editing of context, but these are implementation steps for version 1. we're trying to keep it simple and limit the scope of the feature for the first iteration

marking this as a draft to visually distinguish it in the PR list since it's not ready

This should be ready to review now. PR creation is currently limited to project maintainers (which, in prod, means @ethteck, @nanaian, and @JoshDuMan) and is done from project function pages:

If any of the attempts at the function are at 100% matched, an "Add to PR" button will appear on them. When this button is clicked, the scratch will be added to a basket on the bottom-right. This basket persists across pages for that project, so you can add multiple scratches to build up a larger PR. In the basket is a "Create pull request" button that will:

Create a fork of the repository under your GitHub user if you don't already have one
Create a generated branch like decompme_GEN_asdfgh
In separate commits, update the relevant source files by replacing INCLUDE_ASM macros with the scratch source code (context changes ignored for now)

If the match involved forking, then parent scratch owners will be added as co-authors

Create a draft pull request

Example generated PR here


Create a sha256sum task

Need to be able to compute checksums. Could use alpine:latest for this?

Not required at the moment.


Update browser-automation.md

test in the example does not pass with an exception:
cucumber.runtime.CucumberException: Arity mismatch: Step Definition 'geoorg.Stepdefs.checkTitle() in file:.../cucumber-selenium-java8/target/test-classes/' with pattern [^the page title should start with "(.*)"$] is declared with 0 parameters. However, the gherkin step has 1 arguments [cheese].

Hi @georgeglistru,
Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾
In return for this generous offer we hope you will:

✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
ℹ Don't feel obliged to help, just do what you can if you have the time and the energy.
🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber


Disabled days

How disable any day?

Hey @samiro, right now there isn't a way to disable individual days, only minimum and maximum dates can be set. I don't see this feature being a priority right now, unless it becomes popular.
As always, pull requests are welcome, but I would advise waiting for events (see #3) to be implemented so we can have a similar APIs.

Ok. Thank you very much.

+1

+1

I just wanted to point out to anyone that's interested in this feature, that I managed to implement this functionality buy implementing onDateChanged, keeping track of the currently selected date and re-setting the currently selected date if the newly selected date should be disabled. The other thing that I had to do was implement a DayViewDecorator which set the disabled day's background to the same color as the calendar background.

Thanks for the suggestion @jsa-nic!

Released in 0.6.0. Disabling days was added to the decorator system.

@dandc87 Can you provide an example for disabling certain dates?
I was trying to do as proposed here: :
`ArrayList enabledDates = new ArrayList<>();
        enabledDates.add(new CalendarDay(new Date(2017, 8, 25)));
        enabledDates.add(new CalendarDay(new Date(2017, 8, 26)));
        enabledDates.add(new CalendarDay(new Date(2017, 8, 27)));
        enabledDates.add(new CalendarDay(new Date(2017, 8, 28)));
        enabledDates.add(new CalendarDay(new Date(2017, 8, 29)));
        enabledDates.add(new CalendarDay(new Date(2017, 8, 30)));
        enabledDates.add(new CalendarDay(new Date(2017, 9, 1)));
        enabledDates.add(new CalendarDay(new Date(2017, 9, 10)));
        enabledDates.add(new CalendarDay(new Date(2017, 9, 11)));
        enabledDates.add(new CalendarDay(new Date(2017, 9, 13)));
        enabledDates.add(new CalendarDay(new Date(2017, 9, 16)));
        enabledDates.add(new CalendarDay(new Date(2017, 9, 19)));

        mcv.addDecorator(new DayEnableDecorator(enabledDates));`

`public class DayEnableDecorator implements DayViewDecorator {
private HashSet dates;
    public DayEnableDecorator(Collection<CalendarDay> dates) {
        this.dates = new HashSet<>(dates);
    }

    @Override
    public boolean shouldDecorate(CalendarDay day) {
        return dates.contains(day);
    }

    @Override
    public void decorate(DayViewFacade view) {
        view.setDaysDisabled(false);
    }
}`

However, it doesn't work and it seems like CalendarDay() is deprecated.
Any help would be appreciated.


Allow to connect to a local PID

This patch allow jmxtrans to connect to a local process using its PID.  This is useful if you want to have a local jmxtrans but don't want to embed it or use a java agent.
I've noticed 2 things  that I want to raise to your attention:

Writer tend to use the server and port in their messages. Those are null in my case. However, they also must be null when jmxtrans is embedded. It seems that jmxtrans-embedded has its own writer. We could probably mutualize them by adding a Server.getIdentity method
tools.jar must be in the classpath to make it works. An attempt to add it can probably be done in the jmxtrans.sh script

[ ]   General Comment
 All changes implemented. It is now fully functional without tools.jar.
[ ]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/model/Server.java:L355-395
 I'm always confused by classloaders, but in this case, it seems that we will still throw a ClassNotFoundException in case we load this on a non SUN JVM. It should run fine as long as we do not try to attach to a local process though.
I'm still thinking about this one. It seems that it does not break backward compatibility, so that's probably good enough to merge.
Any idea ?
 Right now, if tools.jar isn't in the classpath, you get java.lang.NoClassDefFoundError: com/sun/tools/attach/VirtualMachine.
Other frameworks are having pretty much the same behavior. They usually try to guess where is tools.jar to add it to the classpath in their sh. I haven't done it because I was a bit lost in the sh code. So I'll leave that to you.
In case of error, the only other thing we can do is to have a nicer error message. And some documentation (but again, I got lost a bit)
[x]   File: jmxtrans-core/pom.xml:L139-179
 It seems that at the moment, all those different profiles set the same value to toolsjar. So we can inline this property for the moment, and introduce profiles once we know about an actual difference between OS.
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/jobs/ServerJob.java:L47-64
 It seems that we now have the concept of a ServerJob and a LocalJob. Could we reflect this in the class hierarchy instead of increasing the cyclomatic complexity of ServerJob ? Or at least encapsulate the construction of the jmxUrl a bit better ?
 LocalJob doesn't exist. This comment is incompatible with the one on getJmxServiceURL. You need to pick one way.
 Disregard this comment and move the implementation in Server
:+1:
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/jobs/ServerJob.java:L75-107
 Comment is redundant with the line of code below, it should be removed.
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/jobs/ServerJob.java:L75-107
 Comment is redundant with the line of code below, it should be removed.
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/jobs/ServerJob.java:L75-107
 Comment is redundant with the line of code below, it should be removed.
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/model/Server.java:L155-176
 This if is useless, if you remove it you will get the exact same behaviour.
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/model/Server.java:L155-176
 I really dislike methods returning null (there are a lot of them for "historical reasons"). Still, let's try not to introduce new ones. In the case of a local JVM accessed by PID, the hostname of the JVM is the hostname of the machine on which JmxTrans runs. So we have a way to cleanly expose a hostname also in this case. So let's not break the contract unless we really have to.
 I almost did that. However, the port would stay null. This is also why I was talking about getIdentity. That would return host:port or pid@host depending on the use-case
 That would be cleaner, but would also break backward compatibility. So no go...
Still, let's save what can be saved. Host has a meaning in all cases, so it should be @NonNull
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/model/Server.java:L239-245
 Instead of removing this method, we should actually start using it. If we encapsulate the JMX URL here, we can have a ServerJob class that is completely agnostic of where the JMX values are coming from. Much much cleaner ...
 Can do. However, the exception will change since I will need to throw IOException, AttachNotSupportedException, AgentLoadException, AgentInitializationException
 I'm fine with adding additional exceptions to the method...
[x]   File: jmxtrans-output/jmxtrans-output-core/src/test/java/com/googlecode/jmxtrans/model/output/OpenTSDBWriterTests.java:L1-24
 We tend to dislike star imports ...
[x]   File: jmxtrans-output/jmxtrans-output-core/src/test/java/com/googlecode/jmxtrans/model/output/OpenTSDBWriterTests.java:L95-102
 The test of OpenTSDBWriter should have nothing to do with pid. So there should be no reason to update it.
 Precondition in Server are requiring to have a pid or a url. I picked the pid. Removing this will mean removing the preconditions
 Sorry, I did read that too fast. Please disregard this comment :+1:
[x]   File: pom.xml:L834-842
 The group is not aligned anymore with the pluginManagement section.
[x]   File: pom.xml:L476-483
 While it is a good idea to update dependencies, this is not related to your PR (as far as I can see), so it should be part of another PR.
 I know. At first there was a sorting issue that forced me to upgrade. If I create another pull request, you might need to accept it to make this one pass
[x]   File: jmxtrans-core/src/main/java/com/googlecode/jmxtrans/jobs/ServerJob.java:L3-10
 Seems we are adding a dependency to the Sun / Oracle JVM. Not good. We need to at least encapsulate this so that it runs correctly on non Sun JVM.
 :+1:  good catch, we have to isolate this in a tiny class that would only be instantiated if Class.forname("com.sun.tools.attach.VirtualMachine") succeeds. Otherwise, jmxtrans may no longer start on IBM JVM ...

This pull request is now managed with CodeReviewHub. Refresh.

All changes implemented. It is now fully functional without tools.jar.

Looks good enough for the moment. Let's merge. Thanks a lot for taking the time to make all the corrections that were asked ! Adding a page to the wiki to explain how to use this new feature would be nice (you should be able to send a PR to the wiki as well).


chore(bazel): use secure-folder-upload and temp dir

closes #2276
closes #2331
This switches the folder upload from the actions/upload-artifact to our secure-upload-folder @ v1.7.0.
This also strengthens the name of the folder that is uploaded to avoid collisions. Since the path to folder cannot be in /tmp with the secure-upload-folder, the name of the folder being upload was changed from binaries to bazel_builder_binaries_to_upload_to_gh to avoid any potential name collisions and be able to utilize the secure-upload-folder action.

cc/ @laurentsimon


Use a runtime-rng feature flag instead of using OS detection

The current OS detection is breaking my code which uses a custom linux target that doesn't have a libc.
ahash should have zero dependencies when built with default-features = false.

Does target_env help here?

I am using a completely custom target specified using a json config. I don't think it is reasonable to modify upstream projects to specifically handle my custom target.
I would much prefer if this was handled in ahash: there's no reason to pull in getrandom when runtime rng is not required. This is the case for hashbrown's default hasher which uses ahash with no-default-features.

This is the case for hashbrown's default hasher which uses ahash with no-default-features.

If a source of Randomness is available, it absolutely should be used with Hash brown.
I am not clear how to express this sort of thing with features. As far as I know there isn't a way to specifically exclude a feature.
If that is true and If there is no way to detect libc's absence by the target or dynamically, I don't see any alternative besides adding another feature flag which disables it.

The proper way to express this would be for hashbrown to expose a feature to enable runtime rng which forwards to the ahash feature.

We want the best available source of Randomness to be used. If we reduce it to a flag, then it throws it on the user to set it for their platform. This likely means using a default which is the least common denominator, and trying to persuade people to lookup their platform and enable a better source.
I don't like that outcome because the use of aHash is very likely indirect. IE an app depends of a library which in turn uses it. If such a library is to be genetic it cannot make assumptions about the platform, so the app will be unaware and end up getting a poor source of Randomness even though a good one is available.
If you are in control of the custom target, you could specify the SYS or the ABI as "none": https://docs.rust-embedded.org/embedonomicon/custom-target.html
That would be easy to detect in the build.rs.

We want the best available source of Randomness to be used. If we reduce it to a flag, then it throws it on the user to set it for their platform. This likely means using a default which is the least common denominator, and trying to persuade people to lookup their platform and enable a better source.

Randomness for a hasher is only useful in the context of HashDoS. hashbrown specifically makes no promises that the default hasher is HashDoS-resistant. I am only using AHash as the default hasher because it is a generally good hash function and avoids the poor quality of FxHash in certain cases.
Anyone who specifically requires HashDoS-resistance should be using the HashMap types from the ahash crate directly, or use the standard library HashMap.

Yes, agreed. I would like a way to support this for hashbrown without it affecting people using aHash directly unless they opt in.
Perhaps we could do this by splitting aHash into two crates. One which contains the build.rs and wraps another which has the actual code. Then Hashbrown could depend on the inner crate, but anyone just using aHash would get the auto-detection logic.

The runtime-rng flag added by this PR is enabled by default so anyone using the ahash crate directly will automatically have it enabled. However if they import it via hashbrown then it will not be enabled since hashbrown imports ahash with default-features = false.

Yes. But how is another library supposed to use it? If some other hashMap library does want randomness this PR forces those libraries to either be like hashbrown and not provide DOS protection or have their builds fail on platforms where random is not available.
I'm not even sure it really solves the original problem particularly well. The root of the issue is that aHash is using getrandom which declares:
[target.'cfg(unix)'.dependencies]
libc = { version = "0.2.64", default-features = false }

in it's config.toml and you are running on a unix without a libc. So if anything else ever caused getrandom to be pulled in then you're right back where you started. It's not implausible that some other crate in the build somewhere pulls in aHash, rand, or any of the other packages that depend on getrandom.
If instead we address the root issue and in getrandom change it's path in linux to instead call: https://man7.org/linux/man-pages/man2/getrandom.2.html
instead of using libc to read from a file. Then libc is never needed in the first place and aHash, rand, and anything else will work out of the box, and everyone's builds will be faster and binaries will be smaller. (In fact this simplifies the logic a lot because the "wait for init" that the syscall provides is being implemented manually in getrandom.)

Yes. But how is another library supposed to use it? If some other hashMap library does want randomness this PR forces those libraries to either be like hashbrown and not provide DOS protection or have their builds fail on platforms where random is not available.

If another library actually guarantees DoS protection then it should require either runtime or compile-time RNG.

I'm not even sure it really solves the original problem particularly well.

It does solve the problem: it is common in no_std projects (e.g. embedded) to make sure that all of the crates you are using do not depend on std or libc. At the moment, ahash is the only crate which causes a problem for me. This is in fact a regression from #62 since previously the getrandom crate was not imported when the std feature was disabled.

If instead we address the root issue and in getrandom change it's path in linux to instead call

That won't work: you would need to use either libc::syscall (which requires libc) or inline assembly (which is unstable).

If another library actually guarantees DoS protection then it should require either runtime or compile-time RNG.

Libraries don't know how they are going to be used. Because something isn't available on a custom target, doesn't mean it should be disabled when it is available.
The Rust standard library does exactly the same thing. If you look at https://github.com/rust-lang/rust/blob/673d0db5e393e9c64897005b470bfeb6d5aec61b/library/std/src/sys/unix/rand.rs#L13
you can see it grabs the best source of randomness it can for it's maps but if it's not avalable:
https://github.com/rust-lang/rust/blob/5a4ab26459a1ccf17ef5bb4c841d3ae5517b2890/library/std/src/sys/unsupported/common.rs#L35
It just uses a fixed key. So there is no difference with the standard library's builder. If aHash's HashMap wrapper is truly going to be a drop-in replacement for the standard library HashMap it needs to support detection with at least as many targets. If the method hashmap_random_keys were exposed, I would call it. However it is not, so the best option for this is getrandom.
The only reason getrandom is not an unconditional hard dependency is because it creates the build issues you are seeing on unsupported platforms. The problem is your custom target looks like a supported one.
Above I mentioned "target_env". Per the list here: https://doc.rust-lang.org/nightly/rustc/platform-support.html
it appears this is always set for linux to indicate which ABI is used. If you really don't have one, this being left empty, set to "none", or some other particular string should make this situation easy to distinguish. In which case all that is needed is adding it to the clauses https://github.com/tkaitchuck/aHash/blob/master/build.rs#L13 here and in the toml.

FYI, you may be interested in an alternative solution in #97.


Serial Pins design discussion

Right now the Pins trait looks like this:
pub trait Pins<USART> {
    fn setup(&self);
}

It is then implemented for pin pairs using a macro:
macro_rules! impl_pins {
    ($($instance:ty, $tx:ident, $rx:ident, $alt:ident;)*) => {
        $(
            impl<Tx, Rx> Pins<$instance> for ($tx<Tx>, $rx<Rx>) {
                fn setup(&self) {
                    self.0.set_alt_mode(AltMode::$alt);
                    self.1.set_alt_mode(AltMode::$alt);
                }
            }
        )*
    }
}

impl_pins!(
    USART1,  PA9,  PA10, AF4;
    USART1,  PB6,  PB7,  AF0;
    USART2,  PA2,  PA3,  AF4;
    USART2,  PA14, PA15, AF4;
    USART2,  PD5,  PD6,  AF0;
    // ..
);

The problem with this is, that it assumes that a single pin pair can be used for an USART peripheral. However, pins can be mixed, so I could use USART1 with PA9 and PB7.
One approach would be to generate Pins implementations for all permutations (which can go into dozens or even hundreds of impls), but that's a workaround for a design problem in my opinion: Different pins can act as Tx and as Rx for an USART independently. (You could even use multiple Tx pins for the same USART if you wanted...)
My suggestion would be to drop the Pins<USART> trait, and to add TxPin<USART> and RxPin<USART> traits instead.
Here's a quick way to play around with this idea without having to adjust the serial trait:
pub trait TxPin<USART> {
    fn setup(&self);
}

pub trait RxPin<USART> {
    fn setup(&self);
}

impl<USART, TX, RX> Pins<USART> for (TX, RX) where TX: TxPin<USART>, RX: RxPin<USART> {
    fn setup(&self) {
        self.0.setup();
        self.1.setup();
    }
}

impl<Mode> TxPin<USART1> for PA9<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF4); } }
impl<Mode> RxPin<USART1> for PA10<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF4); } }
impl<Mode> TxPin<USART1> for PB6<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF0); } }
impl<Mode> RxPin<USART1> for PB7<Mode> { fn setup(&self) { self.set_alt_mode(AltMode::AF0); } }

What do you think about this? I can create a PR with a concrete implementation if you want.
If dropping the Pins directly is not an option, we could also deprecate it and implement the Serial trait for both Pins and RxPin / TxPin.
cc @rnestler

I think your suggestion sounds good. I'd personally prefer to replace the Pins trait instead of incorporating it into the new design. As far as breaking changes go, this one shouldn't have too big of an impact, and I'm more concerned about making things confusing by keeping around an unnecessary trait.

As far as breaking changes go, this one shouldn't have too big of an impact, and I'm more concerned about making things confusing by keeping around an unnecessary trait.

I agree! I'll try to come up with a good design.
Another thing that was brought up in the Matrix chat: It's currently possible that multiple Tx pins can be used for the same UART (maybe also Rx, but that might cause problems). That could break exclusive access to an UART even though the pins have been moved into the driver that uses the UART. I don't think we can do anything about that, right?

Another thing that was brought up in the Matrix chat: It's currently possible that multiple Tx pins can be used for the same UART (maybe also Rx, but that might cause problems). That could break exclusive access to an UART even though the pins have been moved into the driver that uses the UART. I don't think we can do anything about that, right?

I don't quite understand. Are you saying that using multiple TX pins would be fine on the hardware level but cause problems in the HAL? Or are you saying using multiple pins for the same functions is possible in the HAL API, but shouldn't be possible at all?
In any case, we can model all kinds of behavior, by making alternate functions their own struct. Then you would need to involve the function struct when changing the function of a pin. That way, we can move the function struct to make it unusable for another pin, or change its type state to model whatever behavior we desire.
Check out LPC8xx HAL's SWM API for reference. This code hasn't been cleaned in a while, but it demonstrates how to enforce the LPC800 series' quite complicated pin assignment rules at compile time.

Ah, my assumptions were mistaken. This is what I originally meant to write:

A serial driver (e.g. for a modem using AT commands) could be initialized with two USART1 pins (e.g. PA9 / PA10). Those pins are then captured by the driver using ownership, which creates a sense of safety and isolation.
But there's nothing stopping you from using PB6 (also connected to USART1) and to write to the serial device in parallel from a separate task, right?

...but I missed that the USART1 peripheral itself would already be captured by the driver, so it cannot be shared. That answers my question, there's no problem to be fixed here :slightly_smiling_face:

Do we want to close this and PR #68?

Do we want to close this and PR #68?

Why would you close it? Or did you mean merge it for the PR?

Sorry for the confusion, I meant to merge PR #68 and close this issue.

In that case the question was probably directed at @hannobraun?


WIP:  DIFF 3474 comparision point

Creating this for an easy code diff

Clicked too fast, ignore.


Can't save on Mac after adding threat

Saving seems to work fine when the diagram only consists of dataflow objects. As soon as a threat is added, none of the save options work. The menu option displays a save file dialog even though the json file already exists and overwriting it removes the contents leaving just a JSON null. Using the button in the toolbar displays an on-screen error message which says "Cannot read property 'detail' of null". Removing the threat again does not get rid of the problem.
I am using the Desktop version 0.1.26 on MacOS 10.14.6

I do not have this problem when running TD v0.1.26 on MacOS 10.15. @coillarach could you try opening the sample model and seeing if you get the same problem? Just trying to rule out any problem with filesystem write permissions.
BTW I agree that v01.26 is the latest MacOS release, but it is very old at May 2017. I run the later versions of TD by cloning/ downloading the repo and then 'npm run install' followed by 'npm run start'

I have similar issues on 10.15 after cloning the repo, the selected element are not showed and when it does, when a threat is added, none of the save options works.

@lmcthbe could you detail the steps that you take to reproduce this problem on MacOS 10.15 and cloning the latest TD ? I suspect that I do not see it because I am doing something different that you each time. Or is it because there is an error when running npm install?

@jgadsden After fixing the gyp error when doing 'npm install' it helps TD to work better and element can be added to the diagram.
About the save button, I have done extra testing and I found that I can get the save button not gray out just by clicking outside of the element that I have moved, it looks that the save button does not appears (after moving an element) when a another element or the background is not selected/clicked

Thanks for the info @lmcthbe - I can reproduce this issue from your instructions. It seems the Electron pull-down 'Save' always works even if the diagram save button is still greyed out.  I think this can be corrected in the owasp-threat-dragon-core source code. I can add an enhancement request in that repo.

Great.

Hello @coillarach - the new version 1.0 of TD desktop is released:
https://github.com/mike-goodwin/owasp-threat-dragon-desktop/releases/tag/v1.0
and you should not have this issue any more.
Thanks for your help @lmcthbe