Find Required Reason API usage in Flutter Engine and create Privacy Manifest

Introduced in Xcode 15, Apple is now flagging specific APIs as needed a “Required Reason” to use. All usages of “Required Reason” APIs MUST be outlined in a Privacy manifest. A Privacy manifest is a document outlining how your app/SDK collects and uses a user’s data.
This is also time sensitive (https://developer.apple.com/news/?id=z6fu1dcu).

Starting in fall 2023, when you upload a new app or app update to App Store Connect that uses an API (including from third-party SDKs) that requires a reason, you’ll receive a notice if you haven’t provided an approved reason in your app’s privacy manifest. And starting in spring 2024, in order to upload your new app or app update to App Store Connect, you’ll be required to include an approved reason in the app’s privacy manifest which accurately reflects how your app uses the API.

https://developer.apple.com/documentation/bundleresources/privacy_manifest_files/describing_use_of_required_reason_api?language=objc
https://developer.apple.com/documentation/bundleresources/privacy_manifest_files/describing_data_use_in_privacy_manifests?language=objc
https://developer.apple.com/videos/play/wwdc2023/10060/
We'll probably want to also add testing to scan for Required Reason APIs, so they get caught when added.

I'm still finishing the audit, but the new 0A2A.1 handles the large open question here about how to handle Dart's File timestamp APIs.

I've completed the audit of the engine and all of the third-party code it uses. For iOS release builds, we are covered by:

File timestamps:

C617.1 for app state restoration in FlutterAppDelegate.mm.
0A2A.1 for implementation of the relevant File wrappers.

System boot time:

35F9.1 for various event timing and elapsed time calculations.

Thank you so much @stuartmorgan!  I'll start on the PrivacyInfo.xcprivacy to be included in the Flutter.framework.

Well https://github.com/flutter/engine/pull/48951 didn't totally work:

I misread the NSPrivacyCollectedDataTypes section, I thought it wasn't needed.  Will add and empty one.

I misread the NSPrivacyCollectedDataTypes section, I thought it wasn't needed. Will add an empty one.

Never mind I did include NSPrivacyCollectedDataTypes, the error is complaining about NSPrivacyCollectedDataTypes.
https://github.com/flutter/engine/blob/a565cea256c7bafeaa0c26c2f1b0d66a52692d02/shell/platform/darwin/ios/framework/PrivacyInfo.xcprivacy#L9-L12
It probably wants an empty array, not an array with an empty dictionary.
	<key>NSPrivacyCollectedDataTypes</key>
-	<array>
-		<dict/>
- 	</array>
+ 	<array/>

And the error goes away, but the Privacy Report PDF is totally blank so...

the Privacy Report PDF is totally blank so...

Blank in both Xcode 15.1 beta 3 and Xcode 15.2 beta.  I'm going to close this since we aren't getting errors and the blank report seems like an Xcode bug.  I'll play around with it more and file more issues if there's anything we can do about that.  As far as I know the xcprivacy file is correct based on Stuart's audit.

This is available in 3.18.0-13.0.pre


Unpinning xgboost version

Fix #732
Seems like 1.12.0 has the fix?

Thanks! It should, but we need to first fix #744.  My goal is to patch by end of week!

please rebase and hopefully it works! :)

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

Comparison is base (94ce98c) 90.25% compared to head (58dadc2) 90.25%.

:exclamation: Current head 58dadc2 differs from pull request most recent head 27d2112. Consider uploading reports for the commit 27d2112 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #745   +/-   ##
=======================================
  Coverage   90.25%   90.25%
=======================================
  Files          80       80
  Lines        4677     4677
  Branches      856      856
=======================================
  Hits         4221     4221
  Misses        259      259
  Partials      197      197

Flag
Coverage Δ

unittests
90.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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


Exception when running buildProduct

Hi, trying to build the server I'm getting an exception, please could you guide me to solve this?
/irma/irma_api_server # gradle --version

------------------------------------------------------------
Gradle 4.6
------------------------------------------------------------

Build time:   2018-02-28 13:36:36 UTC
Revision:     8fa6ce7945b640e6168488e4417f9bb96e4ab46c

Groovy:       2.4.12
Ant:          Apache Ant(TM) version 1.9.9 compiled on February 2 2017
JVM:          1.8.0_151 (Oracle Corporation 25.151-b12)
OS:           Linux 4.9.60-linuxkit-aufs amd64

/irma/irma_api_server # gradle buildProduct

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project 'irma_api_server'.
> Could not resolve all dependencies for configuration ':runtimeCopy'.
   > Could not find org.irmacard.api:irma_api_common:0.9.0.
     Searched in the following locations:
         file:/root/.m2/repository/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.pom
         file:/root/.m2/repository/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.jar
         https://credentials.github.io/repos/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.pom
         https://credentials.github.io/repos/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.jar
         https://repo.maven.apache.org/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.pom
         https://repo.maven.apache.org/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.jar
     Required by:
         project :
   > Could not find org.irmacard.api:irma_api_common:0.9.0.
     Searched in the following locations:
         file:/root/.m2/repository/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.pom
         file:/root/.m2/repository/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.jar
         https://credentials.github.io/repos/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.pom
         https://credentials.github.io/repos/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.jar
         https://repo.maven.apache.org/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.pom
         https://repo.maven.apache.org/maven2/org/irmacard/api/irma_api_common/0.9.0/irma_api_common-0.9.0.jar
     Required by:
         project : > foundation.privacybydesign.common:irma_server_common:0.3.0

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 1s

The master branch currently depends on an unreleased version of irma_api_common. Using version 0.8.0 should work, i.e. git checkout v0.8.0.

Hi, ok I'll try that, thanks!

I edited the above reply multiple times, so you might need to refresh the page.

Apologies! I've released the missing version of irma_api_common, so everything should work normally again, including the unit tests.

Thanks!


Chat from DCS allies to Discord issue

When a player seated in an aircraft in DCS chats to "allies", it is not reflected in Discord.
Example:

All lines, except 3rd ("hi from allies) was sent to all. Those were all seen in discord, except "hi from allies":

Messages for allies only don't work with the callback, so the system would have to be changed completely.
I would leave it like that for now, as private / allied-only messages are not meant to be put in the - public - discord. So messages from red to red would be seen by everybody, even blue.

Closed for now.


Remove broken Indivisible link on About Page

Also remove NextGen partner sentence.

https://townhallproject.com/#about
REMOVE
For more, we recommend:
Indivisible's Town Hall strategy resources
AND
We are also proud to partner with NextGen America and other progressive allies in supporting grassroots action to hold our elected officials accountable.

Dead link is still up. Bumping for removal as soon as is convenient.

@nathanmwilliams This link isn't dead as of 2:35am, May 7, Seattle time (it leads to a live page on Indivisible's website). Do you still want this link deleted?

Hmm. Good question. Let me check with them to see if they will keep that link up (it was done for a few weeks).
In the meantime, can you remove the sentence at the bottom of the OUR SUPPORTERS section:
We are also proud to partner with NextGen America and other progressive allies in supporting grassroots action to hold our elected officials accountable.

UPDATE: Talked to the folks at Indivisible. The link issue was that we are linking to "www" -- they've been having DNS issues off and on.
Can you change to link to the more reliable: https://indivisible.org/resource/town-hall-guide

@nathanmwilliams We also have a couple of other links to Indivisible's www domain on the website: https://www.indivisibleguide.com/act-locally/ (this one, specifically). Do you want me to change those out to the .org one as well?

Yeah, if those .org links work, that'd be great. Thank you!

@nathanmwilliams I'm going to switch https://www.indivisibleguide.com/act-locally/ over to: https://indivisible.org/groups. This is the link that the original URL is currently redirecting to. Please let me know if this is incorrect.


Not working with collapsing toolbar layout

Hi Miguel,
First of all great work and thanks for sharing.
Do you have any plans to put searchview working with collapsingToolbarLayout or some hint where i should look to change the code. I see you are overlapping the toolbar and i think this is the problem but I don't want to start digging whitout any hint from you or if you are already working.
Thanks

any update about this problem ?

I am facing the same issue while using it in collapsing toolbar. Do you have any workaround for that?

Hey @mehtakunal may I suggest just using another search view (im currently using lapism searchview), just like google playstore.

I am having an issue with the CollapsingToolbarLayout too.
For some reason part of the Toolbar or CollapsingToolbarLayout are expanding to the entire view blocking the content on the screen.

DrawerLayout
2. CoordinatorLayout
AppBarLayout
CollapsingToolbarLayout
FrameLayout
Toolbar
MaterialSearchView


Blockade around a statistical institution of a government

Guys,
I'm a bit pissed off. So sorry in advance for that but do you realize that you have governmental statistics institution in your denylist ?
How do you make those list
Statbel.fgov.be is the federal institution of statistics in Belgium.

 How is that ending up in your list ? For real! You don't block statistics institution. You need them for any debate in any society at any time in human history.
Thanks in advance

anyone forcing you to deploy these lists? Aside from your foul language you exhibiting an attitude of entitlement perhaps though without any contribution to the code.
Mistakes happens and aside from reporting an issue in a more civilized manner you could always elect to

contribute to the code with a PR
move on to another code that is more satisfactory

And in place of getting an over-sized ego like nothing is your fault and that you are defending some editorial line or anything alike, maybe you could just acknowledge the mistake and be astonished that it have even been there.
So maybe I used a foul language, but we are not talking about code here which could have been a complex line-up of functions or anything alike.
No, we are talking about a list of domains or subdomains that should be tested and tested again before to be put in place.
Because don't forget that those lists are being used also by "lambda" people that only use them through services like nextdns or alike. And this is put in place with the push of a button.
I could easily feel the empathy(that apparently you lack) for those lambda people that would take hours if not days to understand where the problem comes from.
I can easily understand that you are blocking every possible domain or nearly of Microsoft for example including the windowsupdate processes like I've seen today. So as you can see I'm always trying to be impartial and objective as the dev manager team that I am. But don't forget one thing, as you are exhibiting the repetitive behavior that I have been able to watch over the years about among dev and IT's:
The " do we force you to use our product... bla-bla-bla... "
So don't forget that, since you are trying to do an honorable thing with this list, it's often better to just acknowledge mistake when they are obvious like here in place of blaming the user who could be validly mad about something that obviously wrong.  Because what you  are doing there if I should do an analogy is a bit like the policeman that would say to the victim of a rape that she should'nt have put that short skirt, or in another matter, the government lying about the utility of the mask in time of pandemic just because they don't have them. Same principle ms, same dynamics of thought, just other subjects. Delegate the fault to others.
Anyway don't worry next time I will do a change ´s request to the list in the code itself with the appropriate comments.
Best regards


[Event Request] Table 383 "Detailed CV Ledg. Entry Buffer" - OnBeforeInsertDtldCVLedgEntryBuf

Hi team,
In table 383 "Detailed CV Ledg. Entry Buffer" function InsertDtldCVLedgEntry we would like to assign some custom fields to the DtldCVLedgEntryBuf record from CVLedgEntryBuf before code is ran, for this an event before NewDtldCVLedgEntryBuf.Init() would be a good solution:

[IntegrationEvent(false, false)]
local procedure OnBeforeInsertDtldCVLedgEntryBuf(var DtldCVLedgEntryBuf: Record "Detailed CV Ledg. Entry Buffer"; CVLedgEntryBuf: Record "CV Ledger Entry Buffer")

Thanks for reporting this. We agree, and we’ll publish a fix asap, either in an update for the current version or in the next major release. We will update this issue with information about availability.


Permission denied docker-compose up

Info:

Docker version ($ docker --version):  1.12.6
Laradock commit ($ git rev-parse HEAD): c85fd97e0031d29e07249dd5cbf951f78197255f
System info (Mac, PC, Linux): LINUX
System info disto/version: Fedora 25

Issue:
What seems to be going wrong?
When I excused the docker-compose up nginx mysql command, it returns me a permission error denied

Expected behavior:
What should be happening instead?
You should rotate the container

Reproduce:
How might we be able to reproduce the error?

Relevant Code:
 zencompany@localhost  ~/Documentos/projetos/playmu/app/laradock   master  docker-compose up nginx mysql
Recreating laradock_mysql_1
Recreating laradock_applications_1
Recreating laradock_workspace_1
Recreating laradock_php-fpm_1
Recreating laradock_nginx_1
Attaching to laradock_mysql_1, laradock_nginx_1
nginx_1               | nginx: [alert] could not open error log file: open() "/var/log/nginx/error.log" failed (13: Permission denied)
mysql_1               | chown: cannot read directory '/var/lib/mysql/': Permission denied
nginx_1               | 2017/05/14 00:11:36 [emerg] 1#1: open() "/var/log/nginx/error.log" failed (13: Permission denied)
laradock_mysql_1 exited with code 1
laradock_nginx_1 exited with code 1

Hi @atiladelcanton
By default, nginx container will save log at host os ./logs, and mysql container also save data at host os /tmp/mysql, please check these folders is enabled write permission.

@bestlong
My machine does not have these folders

@atiladelcanton
please use this command to up container:
docker-compose up -d nginx mysql

and check log you can use:
// for nginx
docker-compose logs nginx
// for mysql
docker-compose logs mysql

then please provide more info use command:
ls -l ~/Documentos/projetos/playmu/app | grep laradock

i guess your laradock folder, maybe docker no write permission.

I put the laradocke's permission in 777 and the same problem continues.

can you post this command result when container is up:
docker inspect laradock_nginx_1

AFAIK you need to add your user to the docker group on Fedora or run docker-compose with sudo

Also check if SELinux is set to permissive, and then do docker-compose up again. If it's not set to permissive, run setenforce 0 and that should do it!


tests: call go test

will add tests later on
Signed-off-by: Boris Glimcher<EMAIL_ADDRESS>

OK, but I don't see any unit tests however.

baby steps


Get results count using query builder

It would be great if we could get results count using the query builder, currently it seems not to be possible
For example:
const qb = ctx.em.createQueryBuilder(...).where(...);

let result = qb.getResult();
let total = qb.getCount();

await Promise.all([results, total]);

Just use qb.execute()

Yes, currently im doing this:
const total = (await qb.select("count(*) as total").execute("get"))?.total;
const result = await qb.select(...).limit(limit).offset(offset).execute();

but it would be nice and cleaner if we could just use getCount like TypeORM
https://github.com/typeorm/typeorm/blob/7a0f7733f2cf704d60548a6b15d1fa2977a9e401/src/query-builder/SelectQueryBuilder.ts#L1141

I said you should use execute to read the results, there is already qb.count() method. Correct way is:
const res = await qb.count().where(...).execute(); // `res.count` is the count

But sure, we can have getCount() too in v5.


[external] docker live-restore not working

Description
When a container is created with live-restore it is supposed to remain active even when the containerd daemon is stopped. But when the containerd is restarted with a container running in background with live-restore, the container is no longer active in the running process.
Steps to reproduce the issue

mkdir /tmp/html
Ensure /etc/docker/daemon.json has live-restore: true
echo "Hello World" > /tmp/html/index.html
Run docker run --name nginx-test -v /tmp/html:/var/www/html/:ro -d --network host gcr.io/google-containers/nginx
Check for the process with docker ps
Restart containerd process with systemctl stop containerd and systemctl start containerd
Check for the process with docker ps now. The container would be no longer active.

Describe the results you received and expected
docker ps

Expected:
Where it should show the
CONTAINER ID   IMAGE                            COMMAND   CREATED        STATUS        PORTS     NAMES
61cb4e0e5e9c   gcr.io/google-containers/nginx   "nginx"   18 hours ago   Up 18 hours             fb4edff8

What version of containerd are you using?
1.6.0_rc.3
Any other relevant information
runc version 1.1.0
Linux 5.10.98+ #1 SMP Tue Feb 8 18:42:19 UTC 2022 x86_64 Intel(R) Xeon(R) CPU @ 2.20GHz GenuineIntel GNU/Linux

Show configuration if it is related to CRI plugin.
No response

Please make sure that dockerd has enabled the live-restore. You can check the last line of output of docker info.

Client:
 Context:    default
 Debug Mode: false

Server:
 Containers: 1
  Running: 1
  Paused: 0
  Stopped: 0
 Images: 1
 Server Version: 20.10.12
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: systemd
 Cgroup Version: 2
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 io.containerd.runtime.v1.linux runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 943ca856ad70358f45e8febce71dd4c931f424ca
 runc version: 067aaf8548d78269dcb2c13b856775e27c410f9c
 Security Options:
  apparmor
  seccomp
   Profile: default
  cgroupns
 Kernel Version: 5.10.98+
 Operating System: Container-Optimized OS from Google
 OSType: linux
 Architecture: x86_64
 CPUs: 1
 Total Memory: 3.593GiB
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  <IP_ADDRESS>/8
 Registry Mirrors:
  https://mirror.gcr.io/
 Live Restore Enabled: true

Please make sure that dockerd has enabled the live-restore. You can check the last line of output of docker info.

It is enabled. This is a part of our validation test suite.

Please make sure that dockerd has enabled the live-restore. You can check the last line of output of docker info.

It is enabled. This is a part of our validation test suite.

OK. Now it is enabled. And could you mind to re-test it again? I think you doesn't restart after apply the step 2 you mentioned

Please make sure that dockerd has enabled the live-restore. You can check the last line of output of docker info.

It is enabled. This is a part of our validation test suite.

OK. Now it is enabled. And could you mind to re-test it again? I think you doesn't restart after apply the step 2 you mentioned

Retested still failed.

sudo docker ps
CONTAINER ID   IMAGE     COMMAND   CREATED   STATUS    PORTS     NAMES

➜  /tmp docker run -d busybox sleep 1d
7406daaf96bbe7cdbe88295c87f50e6e4b6dc32e5384010e2b689472fe95f644
➜  /tmp docker ps
CONTAINER ID   IMAGE     COMMAND      CREATED         STATUS        PORTS     NAMES
7406daaf96bb   busybox   "sleep 1d"   2 seconds ago   Up 1 second             happy_joliot
➜  /tmp docker info | grep Restore
 Live Restore Enabled: true
➜  /tmp sudo systemctl restart containerd
➜  /tmp docker ps
CONTAINER ID   IMAGE     COMMAND      CREATED          STATUS          PORTS     NAMES
7406daaf96bb   busybox   "sleep 1d"   23 seconds ago   Up 23 seconds             happy_joliot
➜  /tmp

Maybe you should check your container log or dockerd log.

still failing on my system. The last working version is 1.6.0_beta.3

Alright. I reproduced it in my local. I am investigating it. Thank you!

➜  containerd git:(main) sudo cat /run/containerd/io.containerd.runtime.v2.task/moby/48152c840db97bb6ee8792e8bf3f422a300410da7cf83e655c585d060297406b/runtime
runc%

The runtime is runc and cause that "failed to resolve runtime path" error="invalid runtime name runc, should be either format like io.containerd.runc.v1 or a full path to the binary".

Closed by #6536. Thanks for the report.


Fix Host hard power off command

Thank you for your suggestion.
Since we manage all of our code through a Gerrit instance we are unable to accept GH PRs. Please contribute this change via the Gerrit server.


Add NeqAssign utility Trait

Adds a trait that makes updating values in changed and update and returning a reasonable ShouldRender value more terse.
Addresses https://github.com/yewstack/yew/issues/631

With the existence of https://github.com/hgzimmerman/yewtil, which contains the same content present in this PR, there really isn't a reason to keep this PR open.


Issue #2194: Implement Operator + 0 and Operator + None

Summary
Adding 0 or None to an Operator instance now returns the instance unchanged.
Details and comments
This allows the use of Python's sum function on Operator lists without specifying an initial value.
Fixes Issue #2194.

 Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

 Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

@mkalis The radd is good, but I'm hesitant to add the special cases for add if its only to make sum work. Out of interest I tested numpy.sum([op1, op2,...]) and that already works with a list of operators and seems to have the same performance so could that be used instead?
My reason for this is because its a bit of a hack as currently op + other is valid for any object other such that Operator(other) is valid (which Operator(0) is not), so it's adding a single special case. If we do add it then it should also be added to the QuantumChannel classes, but it should only be for 0. None should not be included since it is not a numeric type and will typically raise an exception if you try and add None to any other python numeric type.

@chriseclectic I had not considered numpy.sum — that does solve the problem. Thanks!
Another argument for adding 0 + op or None + op was to allow building an operator from scratch, e.g. initializing op = None before a for loop. But I agree that that is a small convenience (and probably better not to do it that way anyway) that does not justify the hack.
Since the only change would now be adding __radd__, I looked at some of the other potential __r*__ functions. Adding the following would be almost as easy as the commutative add: __rmatmul__ (composition, front=True), __rxor__ (for tensor product, implementation using expand), __rsub__ ((-self).add(other)). That would leave only __pow__ and __truediv__ without a corresponding __r*__ function.
Do you think this would be useful? It's also not that difficult for the user to do Operator(other) for all of these (including __radd__). In that case I think we can just close the PR and the original issue.

If the numpy sum works in this case then I think we can close this, do you agree @chriseclectic ?

Ok I'll close this as I think numpy sum is a cleaner alternative than trying to work around python's sum.


Bump acts_as_versioned

Specifies a version that fixes the  "last argument as keyword parameters is deprecated" warning
Fixes deprecation warning when using Ruby 2.7

Coverage remained the same at 93.064% when pulling b29badefe164905777bfc3d8487ba488e49071be on bump-acts_as_versioned into 1254b5625bd10112891640e441c70b790ac4cda4 on master.


Godot 4 Port

Port the changes to Godot 4

I will make a separate repo for this, to keep things simple

I guess I will not close this issue, since there will always be things to port. :) If anything comes up to discuss, we can do that here.

We're good to go for another round of porting changes.
I think, in the future, it would make sense to only do the port once every task in a release milestone is closed, so you don't have to work twice.
For now, there are a lot of changes. I messed up the commits (forgot to pull in your changes first, I was distracted and tired, it was late at night), so there is a mess of revert commits and trying to resolve conflicts while half-asleep. :laughing:
That won't help you much... Anyway, almost everything changed in a way.
In Main, there are big changes, even to the node tree, and how tasks are displayed.
Tasks have the added colour picker thingy, and styles to match
Notes.gd now prepares individual notes with colours and the Note itself (both the scene and the script) changed to accomodate task colours. There are some new icons, and also updated demo tasks and notes....
So it's a lot.
Because there is a colour palette diffrence between Godot 3 and 4, I suggest porting from just use commit 1d928aac, which has the correct colours for Godot 4 in the colour picker (but not the defaults, and the Node StyleBoxes, those need to be updated).
I won't be pushing any more changes, in fact I am planning to put a release tag on the last commit, if you like how things are now. If you have any issues/questions,IU'm always around. :)

Removed this task from the milestone. It can be a "rolling" issue, since porting will always be necessary.


Don't require refout to produce marker

The behavior of MSBuild is at odds with the design discussed by @jcouv in dotnet/project-system#2254. In the design, any project that references a ref assembly will create an up to date marker in it's intermediate output path. Currently only projects that produces ref assemblies and reference ref assemblies create the up to date marker. This isn't sufficient because if you have:
ClassLibrary1 -> ClassLibrary2 -> ConsoleApp1 (where CL1 and CL2 produce ref assemblies)
Then ClassLibrary2 will produce a marker but ConsoleApp1 won't. So there's nothing to compare ClassLibrary2's marker against.
@rainersigwald it looks like just removing the check for producing a ref assembly should be sufficient, what do you think?

Yes, where does it need to go/what approvals do we need to get? Thanks!

Assuming we take #2182 to 15.3, this should be fine--I changed the target branch for you. I'll drive with the team.
You can patch your VS's copy of this file with this change to unblock testing.

@rainersigwald I just pushed another update. I realized as I was working on the project system changes that we should create the marker if the project produces a ref assembly or if it consumes a ref assembly. In the case above, if ClassLibrary1 doesn't produce a marker, then if only its implementation dll changes ClassLibrary2 won't know that it needs to rebuild to copy the new dll and touch its marker.

I just pushed another commit because the Touch tasked was wrapped in a target that only runs when there are copy local references.

@dotnet-bot test Windows_NT Build for CoreCLR please
Odd, I don't recognize failures in
Microsoft.Build.UnitTests.GenerateResource_Tests.InProc.RequiredTransformations.NothingOutOfDateExceptReference
Microsoft.Build.UnitTests.ReadLinesFromFile_Tests.WriteLinesWriteOnlyWhenDifferentTest

@panopticoncentral Do you still need this change if you add the (impl of ref) to (impl of this project) comparison? Won't that cover your original scenario as well?

No, I don't think so. Let me walk through it, though, in case I've got it wrong. Given:
ClassLibrary1 -> ClassLibrary2 -> ConsoleApp1
Where ClassLibrary2 references ClassLibrary1 and ConsoleApp1 references only ClassLibrary2. And both ClassLibrary1 and ClassLibrary2 output ref assemblies.
So we do a full build, touch the implementation of ClassLibrary1, and then build.

ClassLibrary1 is out of date, so it builds ClassLibrary1.dll. However, since the interface didn't change, ref\ClassLibrary1.dll is not touched.
ClassLibrary2 is out of date because input ClassLibrary1.dll is now newer than output ClassLibrary2.dll. So it builds. However, since all inputs of CoreCompile are up to date (since ref\ClassLibrary1.dll isn't newer than ClassLibrary2.dll), we don't rebuild ClassLibrary2.dll or ref\ClassLibrary2.dll. We just copy the updated implementation of ClassLibrary1, and touch ClassLibrary2.marker.
ConsoleApp1 is out of date because input ClassLibrary2.marker is now newer than output ConsoleApp1.dll. So it builds. However, since all inputs of CoreCompile are up to date (since ref\ClassLibrary2.dll isn't newer than ConsoleApp1.dll), we don't rebuild ConsoleApp1.dll. We just copy the updated implementation of ClassLibrary1. There's no marker for ConsoleApp1.

The problem in this situation is that now if you do another build, ConsoleApp1 will fail the up to date check and copy ref\ClassLibrary2.dll again, because ClassLibrary2.marker will always be newer than ConsoleApp1.dll. If ConsoleApp1 had a marker, though, then we would just compare the markers and know it was up to date.
Or am I missing something?

There may an additional problem, and I wanted to check with you. In FindReferenceAssembliesForReferences, it looks like the task creates an UpToDateCheckInput item for the marker of a ref assembly reference. I think this a problem because my understanding of UpToDateCheckInput (which I'm also working on implementing) is that those items are supposed to be checked against all of the outputs. Whereas markers are only supposed to be compared to markers. So it looks like we shouldn't create an UpToDateCheckInput item and up-to-date checkers are just going to have to special case the marker.
Do I have that right, or am I misunderstanding things?

@panopticoncentral That is a correct understanding of the initial implementation, but I believe I already fixed it in #2167 here :)
In addition, I don't think it mattered because UpToDateCheckInputs was read after evaluating the project but not after a design-time build, so updating it within a target was ignored.

Awesome! Correct, it doesn't matter for the old system, but in the new system we're gathering from design-time builds as well, so it would matter there.

Oh, good to know . . . and better behavior!

Per @AndyGerlicher's request, I'm going to take this to a feature branch and run RPS tests on it (since it involves extra I/O for every project).

RPS looks good. I resolved merge conflicts, this can go in after CI passes.

@dotnet-bot test OSX Build for CoreCLR please


Allow page member to be required

This allows the page size member to be required, but still defaults
it to failing the build when it is.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Spec needs updating to say it SHOULD NOT be required: https://awslabs.github.io/smithy/spec/core.html#paginated-trait


Azure Auth: http://localhost:5000/?error=server_error&error_description=Error+getting+user+email+from+external+provider

Bug report
When I login using Azure Auth for a newly created Azure App Registration, I get this error
To Reproduce

Register a new app registration in azure portal, giving access to organisations and personal accounts
Register the supabase redirect uri as a "Web" redirect uri (in azure portal)
Create a secret for this new app registration
enable azure auth in supabase, pasting in the client id and secret of the new app registration
Invoke:

const {user, session, error} = await supabase.auth.signIn({
            provider: 'azure',
        })

That calls https://rlgaaitetwbkhisssmkq.supabase.co/auth/v1/authorize?provider=azure
Which returns with a 302 to https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=16d4a427-b73c-45fc-b04f-9545e7273f21&redirect_uri=https://rlgaaitetwbkhisssmkq.supabase.co/auth/v1/callback&response_type=code&scope=openid&state=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJle.......
Which returns with a 200 and pops the Microsoft Auth dialog, asking for me to grant permission to the app.  Note, it does not ask me what Microsoft identity to use, I assume its using the identity I am using to log into Azure Portal.
I grant permission, then it seems to load https://login.live.com/Me.htm?v=3
Then it seems to load https://login.live.com/oauth20_authorize.srf?client_id=16d4a427-b73c-45fc-b04f-9545e7273f21&scope=openid&redirect_uri=https://rlgaaitetwbkhisssmkq.supabase.co/auth/v1/callback&response_type=code&state=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ....&uaid=95c5d4c9a43f406ca88c5b1b22ea432a&msproxy=1&issuer=mso&tenant=common&ui_locales=en-GB
Then it loads https://account.live.com/Consent/Update?mkt=EN-GB&uiflavor=web&id=293577&ru=https://login.live.com/oauth20_authorize.srf?uaid=95c5d4c9a43f406ca88c5b1b22ea432a&opid=A353DB724539E16F&opidt=1640691597
Eventually we get to https://login.live.com/oauth20_authorize.srf?uaid=95c5d4c9a43f406ca88c5b1b22ea432a&opid=A353DB724539E16F&opidt=1640691597&res=success&route=R3_BAY
which returns a 302 to https://rlgaaitetwbkhisssmkq.supabase.co/auth/v1/callback?code=M.R3_BAY.f96be895-105c-c9f1-5462-d0b31af76746&state=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.ey....
Invoking https://rlgaaitetwbkhisssmkq.supabase.co/auth/v1/callback?code=M.R3_BAY.f96be895-105c-c9f1-5462-d0b31af76746&state=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.ey... returns a 302 to http://localhost:5000/?error=server_error&error_description=Error+getting+user+email+from+external+provider

Expected behavior
I have done the same steps for Google Auth, and it worked, expect the same to happen for Azure auth.
Also, I have successfully logged in using Azure using similar steps using the Microsoft Native javascript sdk for auth, @azure/msal-browser
I am aware of https://github.com/supabase/gotrue/pull/54 , but two things on that.  The first is: I was not presented with an option to select an email address to login with (presumably because I am already logged into azure portal?) but even if I was, the second point is that I don't expect my customers to login using this weird azure specific email address.  I expect them to login using their "usual" azure identity.

I also have this issue

I was able to fix this by using common instead of the tenant_id as suggested by the docs.
https://login.microsoftonline.com/common

Although unrelated to Azure, there is a similar issue for the Keycloak integration which I've fixed by adding openid in the scopes option.
async function signInWithKeycloak() {
  const { user, session, error } = await supabase.auth.signIn(
    {provider: 'Keycloak'},
    {scopes: 'openid'}
  )
}

It should probably get documented in this integration guide.

I have the same issue right now any way to solve this ?
Error:
/SignIn?error=server_error&error_description=Error+getting+user+email+from+external+provider
My signInWithOAuth returns this error
Here is my oauth call:
const { data, error } = await supabase.auth.signInWithOAuth({
  provider: 'azure',
  options: {
    scopes: 'email',
    redirectTo: 'nativeAddress//--/SignIn',
  },
})

The redirect is fine, anyways it doesn't even get there it fails on the returned data url
I followed the supabase social login docs and made sure all api permissions were on

Hey so there was a report that on some Azure accounts the email is encoded differently. @silentworks do you recall?

@anthonylaflamme We have a theory that the problem is #1130 but have no way of testing. Do you mind contacting<EMAIL_ADDRESS>and link to this comment so we can discuss if we can find a way to test this with you?

@anthonylaflamme Yes a support ticket on the dashboard would work too. Just mention this issue in that ticket so we know it's you.

@anthonylaflamme Were you able to figure out a solution for this with the Supabase team?

@karan2801 wasn't able to test got assigned another task at my workplace. I know i'm coming back on this in about a month. Although I was using a personal free account for test so I don't know if that is why it was causing issue.

@

I have the same issue right now any way to solve this ?
Error:
/SignIn?error=server_error&error_description=Error+getting+user+email+from+external+provider
My signInWithOAuth returns this error
Here is my oauth call:
const { data, error } = await supabase.auth.signInWithOAuth({
  provider: 'azure',
  options: {
    scopes: 'email',
    redirectTo: 'nativeAddress//--/SignIn',
  },
})

The redirect is fine, anyways it doesn't even get there it fails on the returned data url I followed the supabase social login docs and made sure all api permissions were on

I'm facing the same issue, any solution so far?

I have the same issue... try the common login URL but still the same issue

@hf
You mentioned this issue should be reopened if scopes: 'email' doesn't solve the problem.
The problem still exists, and can be reproduced. I can see a lot of people experiencing this. Let me know if you can reproduce this and if this issues should be reopened or a new created.
I'm pretty sure I've checked everything on the setup side and the access is really in granted, but this error still persists
To reproduce

Follow: https://supabase.com/docs/guides/auth/social-login/auth-azure
Sign in with NON personal Microsoft account

During sign in url should be https://login.microsoftonline.com, NOT https://login.live.com
The issue is not reproduced for personal accounts, only for Directory accounts

@zomchak-code I'm experiencing the same. Given #292 from 2021 is still open, I'm confused what the tenant URL field in Supabase is even for..

I have the same issue. please reopen this ticket.

@althaf004 does the comment above not fix it for you?

Without any modifications on our end, sign in with Azure is breaking with ?error=server_error&error_code=500&error_description=Error+getting+user+email+from+external+provider - tried everyhing I could think of and all suggestions across this and similar issues, nothing is working so far.

Reopened, thanks for commenting on this. Looking into it!

Thanks @encima ! This kills Azure signups for us and makes us look quite bad in front of our customers who previously were able to use Azure auth to sign in but now are forced to send magic links as a fall back until this is fixed.

@abs / @dzoba in my case, I initially fixed this by adding the email scope, but then a few days later it broke again, but that time it was as a result of an expired client secret. Might be worth double checking that your secret is not expired either?

@abs / @dzoba in my case, I initially fixed this by adding the email scope, but then a few days later it broke again, but that time it was as a result of an expired client secret. Might be worth double checking that your secret is not expired either?

We've been using the email scope from the beginning, so no changes there. Our secret was good, but I created a new one just to make sure; still broken.

As an update, the Auth team are aware and looking into the issue currently; it was not an easy one to reproduce but it is being actively worked on!

@dzoba Just to make sure this issue is only impacting the same setups, can you confirm:

You are not using a personal Azure account
You do not have a custom tenant
Your Entra URL is https://login.microsoftonline.com/common

Thanks!

@dzoba Just to make sure this issue is only impacting the same setups, can you confirm:

You are not using a personal Azure account
You do not have a custom tenant
Your Entra URL is https://login.microsoftonline.com/common

Thanks!

@encima the answers here are the same as in our email exchange - same product :D

Hello folks,
We have reproduced and found a reason, although it might not be fun:

Gotrue is right, there is no email
On a new tenant, new users have their UPN set but not their email. Manually editing their properties to add an email allowed a successful signin
Tenants can be configured to allow email as an alternative login but I am unsure if that helps so much here
This can also be an issue for users where the email is present in their profile but the account is not verified (or the domain is not)

For those experiencing this, can you confirm this is the same behaviour you see?
What we know so far

Does not happen with personal accounts - expected as email is required
Does happen with multitenant and single tenant - which means it is likely the individual users or the tenant settings not sending/setting the email

I'm experiencing this issue. I was testing it with a new account that I created at Microsoft (a new tenant). I could try adjusting the settings to see if it works, but it does seem dependent on the tenant or user, which makes it too risky for me to use as a sign in method without providing an explanation to my potential users. Obviously I'd prefer to avoid this. It doesn't seem like there is a fix we could do on our ends to fix this, is that what the team thinks as well?

That is the current thinking (but it is not guaranteed).
From my AD (Entra) knowledge, I do not see a way to configure a tenant to
force/map an email. Entra also handles this with syncing methods and
Personal accounts but, when you allow multi-tenant, it is up to how they
have configured their tenants, it seems.
On Fri, Feb 23, 2024 at 15:56:33, Seth Fenster @.***>
wrote:

I'm experiencing this issue. I was testing it with a new account that I
created at Microsoft (a new tenant). I could try adjusting the settings to
see if it works, but it does seem dependent on the tenant or user, which
makes it too risky for me to use as a sign in method without providing an
explanation to my potential users. Obviously I'd prefer to avoid this. It
doesn't seem like there is a fix we could do on our ends to fix this, is
that what the team thinks as well?
—
Reply to this email directly, view it on GitHub
https://github.com/supabase/gotrue/issues/550#issuecomment-1961370682,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAD6OI5T6ECY3TGKWWIOV3LYVCNZDAVCNFSM533MJ2Z2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJWGEZTOMBWHAZA
.
You are receiving this because you were mentioned.Message ID:
@.***>

In that case, it's entirely possible for this to be a non issue for some users wanting to sign in with their Microsoft account. The trouble is that we won't know until they try and fail. I haven't had a chance to go through the article you linked. Does that contain instructions on what a tenant would need to setup to get it working? If I could at least provide that as a reference I think I'd be comfortable enough to enable AD as a login option.

@encima do we know what caused it to suddenly stop working? It's strange, because azure multi-tenant auth with email was working after the publication of the linked article, which suggests Microsoft did some stuff, but may not know about it.

I have been looking at their changelogs but don't see anything apart from their mention of UPN and emails. Unless there is some new claim or similar that I am missing.
For those with Azure support contracts, I would reach out to them please (and report back!)

Also experiencing this and can't use Entra URL https://login.microsoftonline.com/common because I want the application to only be accessible from a specific organization (in this case a school).
I coded this website around this time last year for a yearly event and was hoping I could re-use it. Hope this gets fixed soon!

Thinking on this some more, this is still a viable login method as the error does point to the issue, which can then be caught and the feedback for the user to set the email can be shown.
I believe some of you are reaching out to Microsoft about this, so please do update here with their response (particularly on what changed recently for this). We will keep this open while it is being tracked

It seems my Azure AD login is not working anymore after the PR "https://github.com/supabase/gotrue/pull/1269 use OIDC ID token for Azure".
My current workaround is to roll back to gotrue 2.101.2 / supabase cli 1.121.1.

I have been experiencing this issue since trying to get azure login to work for my website. Its a new project with a new Azure tenant.
I was able to get Azure to return email and Supabase to create a user by manually changing the url on the input password page. The url string "scope=openid" needs to be changed to "scope=openid,email" before submitting the password.
This behaviour seems to conform with the Azure manual that scope should by minimum include openid. Other scopes such as email and profile can be added in addition.
https://learn.microsoft.com/en-us/entra/identity-platform/scopes-oidc#openid-connect-scopes
After deleting the user in Supabase I tried to set the option in signInWithOAuth to "scopes: 'openid,email'" but it had no effect and the issue persisted as before.
Hope this helps for further troubleshooting.

HI @dabbijak
Thanks for the steps, the email scope is already documented in our main docs here. The issue is with AD not returning the email address in the response (unless it has been manually set by the Org owner or the user themselves) as they are not mapped.
As this is an AD issue, primarily, I would opt to close this until Microsoft is able to respond with a fix. If people disagree with the decision and want to keep it open, please react with 👎 (to save commenting noise) and we can reopen!


feat: add filters for gt labelling and gt script-type

closes #68

The code itself looks fine, but there a some accessibility concerns:

All controls should be labelled, and while the checkbox for toggling all filters has an ARIA attribute, a label legible by everyone would not hurt.
There are many contrast errors, but only few of those have been introduced by this PR.

I would suggest to check with the WAVE browser extension.
Also, lots of Vue warnings are logged to the console about missing props, extraneous non-props attributes and missing translations. This should be investigated.

The code itself looks fine, but there a some accessibility concerns:

All controls should be labelled, and while the checkbox for toggling all filters has an ARIA attribute, a label legible by everyone would not hurt.
There are many contrast errors, but only few of those have been introduced by this PR.

I would suggest to check with the WAVE browser extension.
Also, lots of Vue warnings are logged to the console about missing props, extraneous non-props attributes and missing translations. This should be investigated.

Thanks for the extensive review.

I added a label to the checkbox for toggling all filters in 9639690. As there is no native functionality/slot for this in the PrimeVue component we use, I had to implement it manually. In this move I implemented a wrapper component to avoid/reduce code duplication.

The WAVE Tool seems very nice, but I can't find contrast errors that have been introduced by this PR. Can you specify where you found these?
I will resolve the rest of the contrast errors in a seperate PR.

I also noticed the Vue warnings when I started this project a month ago and got so annoyed by the amount of warnings that I deselected the warnings filter in the console. 😅 I will also try to fix them in a seperate PR.

* The WAVE Tool seems very nice, but I can't find contrast errors that have been introduced by this PR. Can you specify where you found these?

Selected items in the dropdowns have a contrast ratio of 2.55 or lower, while WCAG AA requires at least 4.5 for normal-sized text. In general, the blue is too light to put in on a white or colored background.

Selected items in the dropdowns have a contrast ratio of 2.55 or lower, while WCAG AA requires at least 4.5 for normal-sized text. In general, the blue is too light, especially on a colored background.

Ah, thanks, I see. As we didn't put much thought into coloring yet, these are just default color stylings from the lara-light-blue theme. I will change the theme to a more accessible one when I will work on #91, because I think that a color theme change is a little bit out of scope of this PR. For now, I made the coloring of the "Select all"-Label a variable that will change with the theming.


Get certificate reconciler from networking instead of control-protocol

Follow-up for https://github.com/knative/networking/pull/802
Proposed Changes

Get certificate reconciler from networking instead of control-protocol
Bump all dependencies with hack/update-deps.sh --upgrade
Drop control-protocol as a dependency

Release Note
NONE

/lgtm
/approve

/retest


Add test for latest crash

Testing this proved more difficult than expected due to the fact that the UICollectionView kept triggering a lot of internal logic that required the setup of all of its data sources, reuse identifiers, layout traits etc. I found a way to silence this by overriding the batch update methods.
The test calls 2 render operations - 1 from inside the completion block of the other. Before the fix, the "last rendered" view model wouldn't have been cached at this point, so both render operations would have resulted in nil diffs being generated.
After the fix, the "last rendered" view model is set before the completion block is called, so the 2nd render operation should generate a diff. The test captures all diffs sent to the layout, and asserts on them.
The PR also:

shares the view model utility code created by the diffing tests.
creates mocks for the layout and component registry.
makes sure the collection view is using a mock layout, not a standard UICollectionViewLayout (or it crashes when rendering).

@JohnSundell @rastersize

Current coverage is 93.29% (diff: 100%)

Merging #226 into master will increase coverage by 0.04%

@@             master       #226   diff @@
==========================================
  Files            69         70     +1
  Lines          4743       4773    +30
  Methods           0          0
  Messages          0          0
  Branches          0          0
==========================================
+ Hits           4423       4453    +30
  Misses          320        320
  Partials          0          0

Powered by Codecov. Last update 3d7e52c...fd78b4b

@rastersize Made the changes you suggested and also fixed the crash (I suspect it was caused by https://github.com/spotify/HubFramework/pull/234)


UX Research — Conduct research to understand pain points and desires of Safe users.

This may not be the right place for this type of note, but a few interviews and surveys could reveal some insights that Fractal may already solve and isn't specifically targeting in messaging, or can easily solve with a little work.
Gives us access to entire Safe user base and get an understanding of who could be a potential user that we are currently overlooking in marketing & biz dev efforts.

Closing this issue, it's not actionable to the design or development teams.


Fix #200:  Flesh out the details of the acknowledgement system in JavaDoc

(waiting for feedback before merging)

Updated title since there doesn't seem to be any concern with this direction.


Make JSX fragment pragma customizable

#198

@mourner renamed

The doc is at https://github.com/bublejs/buble-demo.


OpenGL surface via surfman

Fixes/Discussion: https://github.com/gfx-rs/wgpu/issues/450
PR checklist:

[ ] make succeeds (on *nix)
[ ] make reftests succeeds
[ ] tested examples with the following backends:
[ ] rustfmt run on changed code

If surfman works do you think we remove the glutin feature and make it the only option? If surfman is the only way to support the desired API, we probably want to.
I'm asking because I think I might take out the glutin code just to help me think while understanding the code and avoid a bunch of feature gates, even if I add it in again afterwards.

Another question: for the Headless instance type, should create_surface completely ignore the window handle and create a headless surface? That's what I'm assuming the intent is, but I couldn't be sure because it still receives a raw window handle. Is that just to be compatible with the non-headless API?

@zicklag I haven't investigated surfman in depth. My understanding was that it doesn't work on Windows in the way we want (no resource sharing). Also, if the glutin path can work in the way we want (with pure context for the instance), it would still be nice to keep supporting it. So all in all, I would prefer the glutin path to stay, even if it's only partially usable today.

for the Headless instance type

I don't think there should be a headless instance type. Ideally, we'd expose it at the physical device level. One physical device would be "software", while another will be "hardware". When the user creates a surface, they don't know about a physical device, but the API clearly has means to express that different physical devices can work with different surfaces via supports_queue_family (each physical device would only expose one queue family, and you can make it so the headless physical device is only compatible with headless surfaces. I.e. the only point of divergence on the API level would be the way a surface is created: we could have create_emulated_surface versus another create_surface for accelerated surfaces.

So all in all, I would prefer the glutin path to stay, even if it's only partially usable today.

Sounds good.

Ideally, we'd expose it at the physical device level. One physical device would be "software", while another will be "hardware".

I think I get it. I'll see about that then. I'll try to get non-headless to work first.

Also, what is wgl? Is that for windows?

WGL is basically window system interface in Windows for OpenGL. It's what you use to set up the GL context (internally by glutin, or by other means).

On Feb 29, 2020, at 20:40, Zicklag<EMAIL_ADDRESS>wrote:

So all in all, I would prefer the glutin path to stay, even if it's only partially usable today.
Sounds good.
Ideally, we'd expose it at the physical device level. One physical device would be "software", while another will be "hardware".
I think I get it. I'll see about that then. I'll try to get non-headless to work first.
Also, what is wgl? Is that for windows?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or unsubscribe.

So ideally would we want to support glutin and wgl in separate feature flags, or just rely on glutin to do it?

Overall, Windows is least of a priority for GL (so just because surfman has problems with it doesn't mean we can't technically leave it as the only choice): since on Windows we also have D3D11, D3D12, and Vulkan. I would be totally fine if our GL backend was simply not available on Windows, or expose only compute abilities, or had only software implementation.
For your work, try to focus on Linux & Android first and foremost, then macOS/iOS, and only afterwards - Windows. No need to consider a separate WGL code path at this point :)

I think I have the surfman backend mostly in the state that I want it and it compiles, but I haven't yet gotten it to finish linking so I haven't been able to run it yet. I'm assuming that the problem has to do with my build environment, but I'll have to look more into it.

Perhaps, we need to call set_instance somewhere? :)

Oops, I had that in there earlier! I lost it in a refactor. :)

Do you have any idea what's wrong if I'm getting undefined symbols while linking X11:
 = note: ld: error: undefined symbol: XOpenDisplay
          >>> referenced by connection.rs:79 (/home/zicklag/git/other/surfman/surfman/src/platform/unix/x11/connection.rs:79)
          >>>               gfx_backend_gl-d96c34389fc1283e.29fwo7artum5a8yx.rcgu.o:(surfman::platform::unix::x11::connection::Connection::new::h891448e5eadb68ed) in archive /home/zicklag/git/other/wgpu-rs/target/debug/deps/libgfx_backend_gl-d96c34389fc1283e.rlib
....

And more like that.
I've got a weird setup right now because I'm not an admin on the system I'm using to build, but it doesn't have libx11-dev installed, so to remedy that I downloaded the libx11-dev package source and copied the headers into ~/.local/include/x11 and wrote a x11.pc that includes it.
Apparently I did something wrong, but what does the undefined symbol mean? Does that mean I'm not linking X11 in? Like I'm missing a -l flag to the linker?

Yes definitely missing a link to x11 library.

Ah, thanks. Got it to by adding -lX11 -L$HOME/.local/lib to RUSTFLAGS and symlinking my /usr/lib/libX11.so.6 to $HOME/.local/libX11.so. I keep forgetting that -lsomelib won't look at libsomelib.so.suffix.
I don't know why it didn't "Just Work". Maybe surfman doesn't use pkg-config or something.
Anyway, now I've got a panic in surfman to track down, but it's better than a linker error. :wink:

Hey, we're getting close. You have any idea what this means?
[2020-03-02T03:56:52Z ERROR gfx_backend_gl] [API/Error] ID 1 : GL_INVALID_FRAMEBUFFER_OPERATION in glBlitFramebuffer(incomplete draw/read buffers)
[2020-03-02T03:56:52Z TRACE wgpu_core::command::allocator] recycling comb submitted in 3 when 4 is lowest active
[2020-03-02T03:56:52Z TRACE wgpu_core::command::render] Encoding render pass begin in command buffer (0, 4, Gl)
[2020-03-02T03:56:52Z DEBUG gfx_backend_gl::device] create_framebuffer [Renderbuffer(1)]
thread 'main' panicked at 'Error creating FBO: InvalidFramebufferOperation for RenderPass { attachments: [Attachment { format: Some(Bgra8Srgb), samples: 1, ops: AttachmentOps { load: Clear, store: Store }, stencil_ops: AttachmentOps { load: DontCare, store: DontCare }, layouts: Undefined..Present }], subpasses: [SubpassDesc { color_attachments: [0], depth_stencil: None }] }'

The panic is from here.

There is a blit happening, and it complains that one of the framebuffers is incomplete. First of all, I would assume that your code path does not need a blit. With your surface implementation, rendering to a swapchain should be direct, no copies/blits involved.

OK, I found out where the blit is:
https://github.com/gfx-rs/gfx/blob/106cb216b4fc86663783559ae7a45a88608210b0/src/backend/gl/src/queue.rs#L219-L234
It is probably erroring because I do not have a swap buffers call for surfman and I don't know how to do that. I haven't looked into it yet.
But, you did say that you don't think there should be any copying anyway. Is there a way around using the blit in this case?

Here is how OpenGL allows one to render a frame: you draw framebuffer(0), and you render, done.
Here is how gfx-rs allows one to render a frame: you get the next image, then you have/create a view into it, then you create a framebuffer that includes that view. That framebuffer can include other views as well, such as your manially depth texture. Then you execute a render pass that renders into that framebuffer.
If we don't blit, we would require that if a framebuffer contains the swapchain image (in GL), it has to have nothing else. This is not what users expect... So instead, we create a render buffer internally, render into it, and then when it's time to present - we blit into the GL framebuffer(0).
That blit is costy and highly unfortunate. However, surfman approach gives us hope. Looking at https://docs.rs/surfman/0.1.3/surfman/platform/unix/x11/surface/struct.SurfaceTexture.html for example, I see that gl_texture() just returns a texture ID. That means we can expose it to the users as regular Backend::Image types, and have framebuffers mixing them with other images. Which in turn means that we can finally remove the blit.
Needless to say, you are unravelling one hack in GL after another, and it's sadly all connected. Requires heroic effort to defeat, but Rust community will be very happy :)

Thanks for the explanation and the help. I'm glad we've gotten this far. :)
I'll try to process that and look around and show you what I come up with.

@zicklag how is this going? Facing any troubles?

Hey @kvark, thanks for checking back. I actually hadn't had a chance to look at it until last night. I haven't run into any problems so to speak, but I really don't have a grip on the OpenGL API or the Vulkan/GFX API yet. I'm a little confused at where to start.
I did have a question, though. In this code:
            gl.bind_framebuffer(glow::READ_FRAMEBUFFER, Some(swapchain.fbos[index as usize]));
            gl.bind_framebuffer(glow::DRAW_FRAMEBUFFER, None);
            gl.blit_framebuffer(
                0,
                0,
                extent.width as _,
                extent.height as _,
                0,
                0,
                extent.width as _,
                extent.height as _,
                glow::COLOR_BUFFER_BIT,
                glow::LINEAR,
            );

The point is that you are setting our framebuffer from the swapchain as the buffer we are reading from and you set the write or "DRAW" buffer to None which means framebuffer(0)? Then the blit will blit from our framebuffer and to framebuffer(0)?
So how are we going to get around the blit? By creating a surface with surfman and then rendering to that surface instead of framebuffer(0), something that we can only do because surfman abstracts over surfaces and allows us to present surfaces and not just the result of rendering framebuffer(0)? This is a little fuzzy to me. :)

@zicklag

"DRAW" buffer to None which means framebuffer(0)

In glow, binding None means the default framebuffer, so the one associated with the window.
Would be great if you could hop on "#gfx:matrix.org" room instead of here, given the amount of back-and-forth discussion.

So how are we going to get around the blit?

I can describe what I thought could work, before @msiglreith said in https://github.com/gfx-rs/gfx/issues/3133#issuecomment-596108857 that this wouldn't work...
I thought that instead of creating casual renderbuffers and pretending they are OpenGL swapchain (see this code), and then blitting from them to the default framebuffer, we could work with the GL textures representing Surfman surfaces directly and give them to the user in acquire_image, without any blitting to follow up.
I asked @pcwalton for clarification in https://github.com/gfx-rs/gfx/issues/3133#issuecomment-596164336 . In any way, it sounds like we should be using the latest surfman from git master instead of 0.1, and then switch to 0.2 when it's published. Perhaps, that would let us get further?
It may be that we still need a blit. Even if it's the case, it's not worse than what we have. The main goal of your work still stands - to align the OpenGL API with the others.

OK, I think I get it a bit more now. I already ended up switching to the git master surfman because I had a PR for it and based the PR off of master.

Would be great if you could hop on "#gfx:matrix.org" room instead of here, given the amount of back-and-forth discussion.

I can't get on chat because of network restrictions where I'm at so unfortunately we're stuck here. Thanks for sticking with me so far, though. :grin:
I'll probably try to see if I can fix the error we are getting when the blit is and also look around at acquire_image.

After trying to understand surfman a bit more I realized that because devices and contexts are thread-local, the way that I was using a surfman::Device created by the Instance meant that you wouldn't be able to create surfaces from other threads without violating the Starc.
I just pushed an update that makes the Instance have no references to any thread-local information and instead creates the devices, surfaces, and contexts, on-demand when they are requested, in the thread that they are requested in. If I did this right, this should mean that you can create surfaces in different threads from each-other, but you can't send them over threads or you will get a panic from the Starc.
Now I'm going to look into the update for acquire_image.

@zicklag @pcwalton is the threading model of Surfman described anywhere? I.e. is it just the handle that's thread-local, or the actual device has to live on the same thread?
In the worst case, we'll make it so GL devices can't be moved between threads. Not the ideal situation, but I don't think the current one is better anyway :)

@kvark I was wondering that, too. I checked out the source for surfman::Device and it looks like it is just a wrapper around the connection and the desired adapter:
https://github.com/pcwalton/surfman/blob/ae14cc9afd140bc73e89f3fe8c080f729f09ccd2/surfman/src/platform/unix/x11/device.rs#L12-L18
/// A thread-local handle to a device.
///
/// Devices contain most of the relevant surface management methods.
pub struct Device {
    pub(crate) native_connection: Arc<NativeConnectionWrapper>,
    pub(crate) adapter: Adapter,
}

It looks like it should be cheap enough create new devices when we need them.  With the latest changes I pushed, I use a thread_local! surfman::Connection to back the Instance and when the users wants to enumerate_adapters or create_surface it uses the thread local connection, creating a new connection if one hasn't been created on that thread yet, and then creates a Device, Context, and Surface right there, in the same thread it was requested.
While you won't be able to transfer Surfaces over threads, at least you can still create them in multiple threads.
The thing I'm not sure about is whether or not Device's created with different connections will share resources, which would be required for that setup.

I just realized that the PR is targeting hal-0.4. Is it intended? If this is breaking the API, it should definitely go to master.

I don't know, actually. I haven't actually broken anything that was already there yet, just added a new feature and implementation for surfman, as well as a couple extra functions for glutin. As far as I know that shouldn't be breaking.

Ah ok, that's fine then. We typically only target the past branches with patches, or at least small well-defined features. This one is a big and not well defined :)

OK, so I made some progress understanding the way some of this is put together. I finally realized that you need the framebuffer from the surfman SurfaceInfo and to gl.bind_framebuffer() in order to actually render to the surface. I had completely missed that. I also think I fixed a problem that would have occurred with the pixel format because I didn't have an alpha channel. Things are coming together in my head a bit better.
I've come to the conclusion that, right now, even though we would rather not have the blit in there, it should be working with the blit there and something else must be broken, I just haven't found out what it is yet. After finding the surfman surface framebuffer object I still don't know exactly where to put that framebuffer. In the code I put it here, but I'm not sure that is right:
https://github.com/gfx-rs/gfx/blob/4c9ab0742492800fdc13bed5c4948f367e4b76ee/src/backend/gl/src/window/surfman.rs#L250-L257
When I try running that I get a different invalid operation:
[2020-03-12T01:46:14Z ERROR gfx_backend_gl] [API/Error] ID 1 : GL_INVALID_OPERATION in glFramebufferRenderbuffer(window-system framebuffer)
thread 'main' panicked at 'Error allocating memory buffer InvalidOperation', /home/zicklag/git/other/gfx/src/backend/gl/src/device.rs:549:21

Any hints on that?

I've come to the conclusion that, right now, even though we would rather not have the blit in there, it should be working with the blit there and something else must be broken, I just haven't found out what it is yet

That sounds about right. Glad we are on the same page!

After finding the surfman surface framebuffer object I still don't know exactly where to put that framebuffer

Wait, surfman gives you a framebuffer? My expectation (or wish?..) was that it gives you a GL texture, which we'd have as an attachment to our framebuffers. If it gives you a framebuffer, you can't configure it (i.e. add any attachments), you'd just be binding it to the DRAW_FRAMEBUFFER bind point and issuing a blit from another framebuffer (that we manage, like the current implementation does) that has an attachment texture with the frame pixels.

you'd just be binding it to the DRAW_FRAMEBUFFER bind point and issuing a blit from another framebuffer (that we manage, like the current implementation does) that has an attachment texture with the frame pixels.

Ah, so in that design, the one we would prefer to avoid, we would blit on to our surface framebuffer from surfman instead of blitting onto framebuffer(0).

So I had read this:
https://github.com/pcwalton/surfman/blob/ae14cc9afd140bc73e89f3fe8c080f729f09ccd2/surfman/src/device.rs#L190-L196
    /// Returns various information about the surface, including the framebuffer object needed to
    /// render to this surface.
    ///
    /// Before rendering to a surface attached to a context, you must call `glBindFramebuffer()`
    /// on the framebuffer object returned by this function. This framebuffer object may or not be
    /// 0, the default framebuffer, depending on plat

I was thinking you had to bind the framebuffer, so that is what I needed to get to render to the surface. It seems you can create a texture, but the textures are read only and doesn't work on the window widgets?
    /// Creates a surface texture from an existing generic surface for use with the given context.
    ///
    /// The surface texture is local to the supplied context and takes ownership of the surface.
    /// Destroying the surface texture allows you to retrieve the surface again.
    ///
    /// *The supplied context does not have to be the same context that the surface is associated
    /// with.* This allows you to render to a surface in one context and sample from that surface
    /// in another context.
    ///
    /// Calling this method on a widget surface returns a `WidgetAttached` error.
    fn create_surface_texture(&self, context: &mut Self::Context, surface: Self::Surface)
                              -> Result<Self::SurfaceTexture, (Error, Self::Surface)>;

We were supposed to be writing to textures or do you write to the texture through the Context and only read the texture?

From what you are quoting and saying, it does unfortunately look like we have to have our own texture to render to, and then just blit into the surfman framebuffer. Let's at least investigate why it's complaining when we do this. Have the code to reproduce it?

Hmm, that actually contradicts https://github.com/gfx-rs/gfx/issues/3133#issuecomment-596174198 if that discussion was being understood by both parties the same way. I wonder if there is a tiny chance the code/comments you are looking at is just outdated stuff from surfman-0.1. In any way, we need to get the blit path working now, to restore some sanity, and go from there.

Have the code to reproduce it?

Yes. I'm just using the wgpu-rs cube and triangle examples as the test. You'll need to pull the following in addition to this PR:

https://github.com/gfx-rs/wgpu.git
https://github.com/gfx-rs/wgpu-rs/pull/182

Also my in wgpu-rs Cargo.toml file I have the following:
[patch.crates-io]
gfx-backend-gl = { path = "../gfx/src/backend/gl" }
gfx-hal = { path = "../gfx/src/hal" }
gfx-backend-vulkan = { path = "../gfx/src/backend/vulkan" }

[patch.'https://github.com/gfx-rs/gfx']
gfx-hal = { path = "../gfx/src/hal" }

[patch.'https://github.com/gfx-rs/wgpu']
wgpu-core = { path = "../wgpu/wgpu-core" }
wgpu-native = { path = "../wgpu/wgpu-native" }

To test it run cargo run --example cube --features surfman --no-default-features.

I wonder if there is a tiny chance the code/comments you are looking at is just outdated stuff from surfman-0.1.

Maybe, but those docs are not from 0.1, they are from master, and they aren't actually on Docs.rs so they are newer than 0.1. They could still be older than the current implementation, but at the very least they are newer than the published crate.

In any way, we need to get the blit path working now, to restore some sanity, and go from there.

:+1:

I'm able to reproduce the problem on the gfx-rs quad example. Hooking up the whole wgpu-rs for this is a bit too early - we can work within gfx-rs until we get at least anything working. Here is the modification - https://github.com/kvark/gfx/commit/20f6e4da10f7b511d0c0e645a646c6cd79d7f8bc
Spewing the framebuffer status of DRAW_FRAMEBUFFER, which has surfman's FBO in it, gives me 0x8219, which stands for GL_FRAMEBUFFER_UNDEFINED. According to the docs this would only happen if we don't really have a default framebuffer.
This is actually reasonable, since the default framebuffer is a property of the current context. What's missing here is a switch to the GL context associated with the surface we are blitting into. Once I add this, the error goes away, and the blit happens. However, there are more issues to take care of:

we need to signal surfman's context that the frame is ready. I quickly looked for swap_buffers in surfman but didn't find any
the log says we are creating a framebuffer every frame ("create_framebuffer [Renderbuffer(1)]"), which is suspicious
nothing is actually rendered, the screen is black

@zicklag  Would you be able to take it further from here? Or need help looking into any of these?

@kvark Thanks for looking into that! I'll try to take it from here. This is a great learning experience.
I think the surfman version of swap_buffers might be present_surface so I'll look into that.
I'm starting to get a better grip on the threads example for surfman which is confusing to me, but seems to indicate that I'm missing some essential steps such as binding the surface to a context which I recently wasn't doing.

we can work within gfx-rs until we get at least anything working

Sounds good.

@zicklag where are we now?

I haven't had time to look into it again yet. I've been a bit busy. I might get to looking at it again in the next few days.

There is a lot of progress here, and a great deal of investment. I just don't want it to be lost :)
Even if some stuff doesn't work as expected, we can land the code.

Its up to you if you want to land this and start another PR for continued work, but I'm still very interested in getting this working. :)
Glad to have interest in getting this in, though, I've had larger efforts than this that didn't end up getting into other projects before. :smiley:

Just an update: I did some more looking into it and after trying some stuff out, I'm still having trouble getting anything to show up on the screen. I'm going to do some more experimentation.
Also as far as the log about the framebuffer creation, it seems that the function that is logging that message is the Device::create_framebuffer which I don't think is called from anywhere inside the backend, which makes me think that it might be the demo's rendering code that is making that call over  and over.
Anyway, not much progress just yet, but I think we're close. :slightly_smiling_face:

@zicklag great, thanks for the update! Let us know if there is anything of a concern, or a blocker.

So I just did a test to make sure I could get an onscreen version of surfman's offscreen example running, to make sure that I could actually get a non-black screen from surfman in the simplest situation. Everything worked fine, which is really good. The biggest thing that I proved out with that was that my native_widget_from_rwh ( raw window handle ) PR to surfman actually worked. Now that I'm sure what the simplest situation looks like I'm going to try to look into why things aren't getting to the screen.
There has to be a break somewhere where I'm not setting the right context to current or something like that I think. Otherwise, it is actually very simple to display the surface, it is just easy to get lost in the abstractions in the backend implementation.
Sorry for the super slow progress, but I'll keep working at it. :)

Amazing news, @zicklag , let's get this work wrapped up!

Whoa! Praise the Lord, it just worked! The whole automatic "make context current" when dereferencing the GlContainer was making it break, probably because that context that it is making current should only really be used when evaluating the devices capabilities ( if I understand correctly ).

There are all kinds of TODO in the code all over the place, but it was kind of already a little like that. ;)
I'm going to look into that assertion that I commented out of WGPU to see if I can fix that now.

I made a fix for the failing assertions, but I don't know that it was technically correct. It seems to work so far. I just changed the default min storage buffer alignment to 256 instead of 1024.

@zicklag greatness! let's make CI happy, and be sure to have surfman feature tested on Travis.
I'll re-review the code tomorrow.

let's make CI happy, and be sure to have surfman feature tested on Travis.

Good idea. I'll check out the CI failures.
A couple other tasks:

There is a panic that I need to fix that happens when dropping the Instance ( or surface or something like that ), but I know what the issue is there and it will be easy to fix.
I don't have a wayland device to test on, or I don't know how to test Wayland anyway. I'm running PopOS and I think that it might be running an X11 protocol adapter on top of Wayand or something like that.

I didn't figure out how to get the surface size from a Wayland raw window handle: https://github.com/pcwalton/surfman/pull/61/files#diff-292e583dbc847cf34f3a0d0a4ba31801R189-R192

I wish I knew the answer. 10 minutes of searching didn't reveal it :(
Calling for @msiglreith - do you know how this is supposed to work? How does one figure out the Wayland surface dimensions?

@kvark there is no real size, it's determined on swapchain creation - see also the notes for the wayland extension https://renderdoc.org/vkspec_chunked/chap32.html#_wsi_surface

OK, did a good chunk of cleanup. I fixed the panic that was happening upon surface drop. I managed to get the make pristine on Linux, but it is failing in CI on windows and Mac. There must be some failure to feature gate the surfman and glutin crates out of the Windows and Mac builds. I'll have try fix that.
The examples don't work with the glutin backend anymore because we removed the gl feature specific code as the API is now properly aligned with the other backends ( yay! ). Technically I think the glutin backend still works fine if you conform to its API divergence ( I was using the glutin backend with wgpu earlier and it worked ).
I might have to look at WASM, too because that is failing CI as well.
Other than CI, is there anything else I should do?

That's wonderful news! I think, as long as surfman path works, we can afford to (temporarily?) lose glutin path, given that the new path has no API hacks, and we can finally get it rolling on wgpu and friends. So CI is the only thing that needs to pass here, and please make sure surfman path is tested on it.
Also, it would be good to squash some commits. Ideally, there should be a linear progression with each step compilable by itself. If it's impossible, we should just squash everything.

The surfman path will be tested now with the examples depending on it so I think that should be all we need. I'll do an extra check of the Makefile to make sure I should't have any extra changes in there.
Probably almost none of those commits other than the last few actually work so I'll just squash the whole lot of them.

@kvark Do we need to test the gl backend on Mac? I don't think its ready yet, so we skip it in CI for Mac for now right?

@zicklag what's missing in mac? I thought surfman would abstract this away.
Testing mac GL would be useful, as a backup for mac Metal.

At the very least it is missing an implementation for surfman_surface_from_raw_window_handle, which I implemented for Wayland and X11, but I can't test or implement for Mac. I'm not sure if there is anything else that isn't implemented, but actually I think you are right that surfman should handle the large majority of it at least.

Is that in a surfman PR? I can try adding this and testing it.

also, if it's on the surfman side, we don't have to block on it here

Is that in a surfman PR? I can try adding this and testing it.

Yep: https://github.com/pcwalton/surfman/pull/61

also, if it's on the surfman side, we don't have to block on it here

No, but it will fail CI until surfman works. Oh, maybe not actually because it should still compile even if it fails at runtime with unimplemented!(). OK, I'll see if I can get it to compile on Mac then, even if it doesn't run yet.

I'm removing glutin from any tests for now if you are fine with that. We could bring it back later if we want.

It looks like surfman doesn't support iOS unfortunately. That leaves us some sort of decision to make.
Currently the big gain of the surfman implementation is the unified API such that we don't have to treat the GL backend differently than the other backends like we used to. The examples have already been adapted to this. If we have to use glutin for iOS, though, we would have specific handling for iOS, like we needed before surfman. I'm not sure if it is possible to make the glutin backend conform to the desired API similar to the surfman backend, but as I understand, the point of surfman was to give us an API advantage when threading that we wouldn't have with glutin.
Do we even need GL on iOS, really, though? Every iOS device is going to have metal on it right? In that case it seems like there isn't really any point to GL on iOS. It should be less performant to use GL, too.
I'm thinking we just don't test GL on iOS and target iOS with the metal backend.

Yes, let's proceed without GL on iOS.

On Mar 28, 2020, at 19:34, Zicklag<EMAIL_ADDRESS>wrote:

It looks like surfman doesn't support iOS unfortunately. That leaves us some sort of decision to make.
Currently the big gain of the surfman implementation is the unified API such that we don't have to treat the GL backend differently than the other backends like we used to. The examples have already been adapted to this. If we have to use glutin for iOS, though, we would have specific handling for iOS, like we needed before surfman. I'm not sure if it is possible to make the glutin backend conform to the desired API similar to the surfman backend, but as I understand, the point of surfman was to give us an API advantage when threading that we wouldn't have with glutin.
Do we even need GL on iOS, really, though? Every iOS device is going to have metal on it right? In that case it seems like there isn't really any point to GL on iOS. It should be less performant to use GL, too.
I'm thinking we just don't test GL on iOS and target iOS with the metal backend.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

After thinking a bit about using this backend in wgpu I'm feeling like we should try to get the GL backend to compile without having to specify any target-specific features such as wgl. Otherwise I don't know that it can be compatible with wgpu's concept of not needing to specify features for the backend. I'll look into that.

On another note, CI is passing! :tada:

Correct - we should be allowing GL backend selected on platforms that support surfman, i.e. Linux X11, Wayland, Android, and macOS.

Got it.
With some help on the forum, I just found a way to very nicely encapsulate #[cfg] checks into "aliases" that we can use in code so that we can just #[cfg(surfman)] instead of having to do stuff like #[cfg(all(not(target_arch = "wasm32"), feature = "surfman", unix)))] so I'm going to go through the code and make it so that on any platform you can include whatever features you want without hitting compiler errors. If the feature is not supported on the platform, then it will just not be used.
The perfectly addresses both mine and @grovesNL concerns about the features setup.

I've been occupied all week, but I'll be attempting some work on this this weekend and hopefully will finish or make progress on the feature-less compilation and the rebase and other reviews.

With a some great help on the forum I was just able to release the cfg_aliases crate which greatly simplified our cfg checks throughout the code here. Now in the build.rs we have:
https://github.com/gfx-rs/gfx/blob/693317a043d3ee6d67be8bf0ea6db3c3269c11f3/src/backend/gl/build.rs#L8-L20
That creates aliases that we can then use in our code:
https://github.com/gfx-rs/gfx/blob/693317a043d3ee6d67be8bf0ea6db3c3269c11f3/src/backend/gl/src/lib.rs#L40-L65
That dummy check in particular would be rather verbose if it weren't for the alias, but now we can simply and maintanably manage our cfg checks without adding a bunch of verbose and confusing attributes.
Plus if we decided that we only wanted to support surfman on linux ( not that we would do such a thing ) we could update the cfg alias in one place and have it updated everywhere.
I've still got to update the examples and remove the feature checks from CI and such. I'm not 100% sure it is working right yet, but once I get CI to pass we should be in the clear. :smiley:

@grovesNL I'm having issues with the latest glow for the WASM quad example because in glow 0.4 the UniformLocation type isn't Copy anymore so UniformDesc can't implement Copy either.
https://github.com/gfx-rs/gfx/blob/5cd986dce448f20ce66ab8943484d3f74d38a1e2/src/backend/gl/src/native.rs#L315-L320
If I try to remove the Copy bound on UniformDesc I get issues because it isn't Send either, so it can't be put into an RwLock ( I'm pretty sure that's the cause, anyway ).
For now I just rolled back glow to 0.3-alpha4 again. glow 0.4 does work fine for the non-wasm backends. Do you know of anything I can do to address this?

@kvark @pcwalton Do you have any insight on to whether or not we should be using https://github.com/servo/surfman or https://github.com/pcwalton/surfman here?
I don't know what the relationship between the repos is, but now servo's version is 31 commits ahead of the @pcwalton's, one of which being a method to resize a widget surface, which we are actually going to need. ( There is an open TODO in GL backend code right now ).
I know that servo recently merged @pcwalton's after having been way behind it. Is further development happening in servo's repo?

@zicklag could we remove Copy from UniformDesc and try to fix the problem with Send? We could also wrap UniformLocation in Arc or something else if necessary (at least for now)

@grovesNL  OK, I've worked around it by putting it in a Starc that will make it Sync + Send but it will panic if accessed from outside of its original thread. Probably not ideal and I'm not sure if there is a better way around it, but it will work for now.

@kvark It looks like Mac builds are failing right now because the CI server doesn't have x11 installed. Do you know how to get X11 installed on there or does X11 not even run on Mac? According to the surfman cargo TOML it looks like it isn't excluding the X11 feature for MacOS so that seems to indicate that it could work.
I could add a gl-x11 feature to the examples but it would have to default to off and users would have to enable it manually to support X11 on Linux. It would clutter up the features, which is something we are trying to avoid.
The two ways out of this I can see is to either put X11 on the CI server or tell surfman not to include X11 on MacOS unconditionally ( which if it actually is supported we would not want to do ).

OK, I think it is pretty much ready ( again :smile: ) other than the GL version selection comment above and MacOS/iOS builds which are failing because of the above-mentioned problem with X11.

It looks like Mac builds are failing right now because the CI server doesn't have x11 installed

We don't expect the users to have X11 installed on macOS either.
Can you make it so the x11 feature is auto-enabled for GL but only on Linux?

We don't expect the users to have X11 installed on macOS either.

Yeah, good point. Unfortunately, limitations of Cargo are preventing us from changing the selected features of surfman depending on the target. I'll try to see if there is a way around it, though. I've got one idea I haven't tried yet.

@kvark I just noticed in the surfman source code that the x11 module is actually gated so that it won't be used on macos even though the dependency on x11 ( which is actually what is failing, the dependencies build.rs ) is not gated. This means that all we have to do is gate the dependency as well as the code and I think we will be good.
That should be easy. I'll start on that.

Also @kvark what are you thinking about which surfman to use? Right now we have to use a git dependency already so that doesn't make a difference. Should I try to make a PR for servo/surfman with the changes we need because it is getting some more recent development?
Haven't heard anything from @pcwalton yet.

Looks like servo/surfman is the upstream, and we should be using that. We've just made a gfx-hal-0.5 release, so it's fine to have some git dependencies for a while to settle up and test, until we need to release gfx-hal-0.6.

Almost have surfman ready. Might be done within the next few days. :crossed_fingers: .

@kvark @monocodus doesn't seem to be respecting the nightly-only rustfmt features such as spaces around range patterns. Which rustfmt version should I use?

@zicklag please ignore the bot. It's fresh and we don't yet have a way to configure it properly.

I gave it a quick test and the quad example using the gl feature displays a black screen on my environment (Arch Linux with X11, NVIDIA drivers).
I decided to test the master branch of servo/surfman and the threads example seems to exhibit the same issue. I have reported the problem here: https://github.com/servo/surfman/issues/173
I am guessing both things are related, so I figured I'd mention it in case someone else decides to test this.

@kvark There was that unaddressed note about the default GL version that I couldn't figure out ( actually now that I say that, I can't remember if an incorrect value there might have led to a blank screen like @hecrj is getting ), but otherwise, CI just passed ( though I re-pushed after squashing commits ) and I think Its done!

Unfortunately I just found that the reftests are segfaulting for GL and the makefile wasn't testing GL on CI properly.
I'll have to look into that. I just pushed a commit that addresses all of the points of the review and additionally allows you to run with both the glutin and the surfman features enabled with surfman taking precedence on Linux. This lets you use glutin on Windows and surfman on Linux if you preferred glutin for Windows over wgl.

As the default features being our last change, it would be great to try taking the quad example from hal-0.5 branch, moving it out, and compiling with "GL" feature against your modified code.

I'll try that out.

For some reason it is segfaulting right here:
https://github.com/gfx-rs/gfx/blob/99dc1164bd77bb56344dc79c2e510874ac8c9f34/src/backend/gl/src/info.rs#L126
It is when it tries to get the vendor name from the gl context. There it isn't happening when running the quad example, but it will happen if you re-order the creation of the surface and the enumeration of the adapters.
Any clue what might cause that?

Is SetCurrentContext being called?

On Apr 12, 2020, at 15:54, Zicklag<EMAIL_ADDRESS>wrote:

For some reason it is segfaulting right here:
https://github.com/gfx-rs/gfx/blob/99dc1164bd77bb56344dc79c2e510874ac8c9f34/src/backend/gl/src/info.rs#L126
It is when it tries to get the vendor name from the gl context. There it isn't happening when running the quad example, but it will happen if you re-order the creation of the surface and the enumeration of the adapters.
Any clue what might cause that?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

Ah, thank you, that is definitely the cause.
Apparently I was just getting lucky with the fact that it happened to be working with the order that the current quad and wgpu examples were running certain functions. Now I'm having a little bit of trouble, but I'm not necessarily stuck yet.
The problem is that when I try to run make_current on the context whenever the GlContainer is dreferenced, it just renders a blank screen for some reason. When I don't make current on dereference it errors out somewhere along the line ( unless you get lucky, like I did ealier ) and I need to figure out all of the places in the code I need to make current now. Hopefully that won't be too difficult.

@kvark I found out where to put the make_context_current call to fix the error, but I'm not 100% sure it is the best spot to put it:
https://github.com/gfx-rs/gfx/blob/c4e2c2dac4b4bdab0cfde62b36047de0cc61fda7/src/backend/gl/src/window/surfman.rs#L314-L317
It is the earliest spot that I could find after enumerating adapters that has access to the specific surface that needs to be made current. I tried sticking it in configure_swapchain which is where I felt that it should go, but it panics with an invalid command error.
It seems like maybe we should be making the context current on de-reference, but I still don't know how to fix the black screen that happens when we do that, and I don't know how to go about debugging it. I feel like we can leave it for now and fix it later.

The reftests aren't segfaulting anymore but they are hitting unimplemented!() for Device::create_query_pool(). Do we just leave that for now?
Also, CI isn't set to run the reftests, but there is a Makefile entry specifically for CI reftests. Does that need to be added to the Travis config?

I just tested the following steps and got the quad and colour-uniform examples working on hal-0.5:

switch to this PR's branch
rebase onto hal-0.5
Change the default features of gfx-backend-gl to [] like it was before the PR, to avoid making a breaking change
Update the gfx-backend-gl dependency to include the features = ["wgl", "surfman", "surfman-x11"]
Run the examples

So as long as I'm not missing something other than the default features that makes this change breaking, I think we can safely backport it to 0.5!

@zicklag what happens if the example tries to use the glutin feature like it does today?

Ah, I get it now, we want to test the old examples to make sure that they still work as a validation of the fact that our changes are still backward compatible. :+1:
I tested it and, good news! The examples work without changes after the latest commit I made to re-add some "unused" imports/exports in the glutin implementation.
That means that the only difference that will be necessary for the backport and the master commit will be to make sure the default features and examples stay unchanged from hal-0.5 for the backported version. :star2: Piece of :cake:.

That's wonderful!
So where do we stand now? Are there known issues blocking the merge at this point?

Not that I know of. Some things such as when the context gets made current might not be technically correct or work in all cases, but that is kind of the story with the whole backend right now. :wink:
I think its ready. If we notice anything else after we can make follow-up PRs.

@zicklag would you want to follow-up with a PR to hal-0.5 that has this code with the default features fixed?

Yeah, I can probably have that PR ready by tonight.