Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically regenerate the files #478

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

Automated changes by create-pull-request GitHub action

@OskarStark
Copy link
Collaborator

What's the clue here? Why does it remove all files?

@staabm
Copy link
Collaborator

staabm commented Nov 30, 2024

the job runs into errors (see "Regenerate files" step), therefore deletes everything but fails at generating new stuff

https://github.com/thecodingmachine/safe/actions/runs/12093074222/job/33723621088


In PhpStanType.php line 64:
                                               
  Error when trying to extract parameter type  

@icanhazstring
Copy link

Debugging the issue. It seems that the issue is coming from the following function imagecreatefromavif. Seems phpstan tries to get the returntype for GdImage but can't find anything.

Digging a little further.

@icanhazstring
Copy link

Ok so - after some more debugging.
Multiple files are loaded but do not have any definition in phpstan functionsMap.

https://github.com/phpstan/phpstan-src/blob/2.0.x/resources/functionMap.php

These are multiple ones. Two things we can do here:

  1. ignore undefined functions in generation and skip them - but output a warning? (wait for phpstan to support them)
  2. add the functions with their definition into the CustomPhpStanFunctionMap.php with all their definitions (which will be a hassle since this will take some time to do, here an example https://github.com/thecodingmachine/safe/compare/master...icanhazstring:fix/undefined-functions?expand=1)

What do you prefer?

@staabm
Copy link
Collaborator

staabm commented Dec 2, 2024

thx for the investigation. could you give me a list of which functions we are talking about?

@icanhazstring
Copy link

I will try and export something 👍

@icanhazstring
Copy link

I hope I got everything:

string(19) "imagecreatefromavif"
string(18) "imagecreatefromtga"
string(25) "openssl_cipher_key_length"
string(12) "pg_lo_import"
string(11) "rnp_decrypt"
string(24) "rnp_dump_packets_to_json"
string(16) "rnp_dump_packets"
string(14) "rnp_ffi_create"
string(15) "rnp_import_keys"
string(21) "rnp_import_signatures"
string(24) "rnp_key_export_autocrypt"
string(25) "rnp_key_export_revocation"
string(14) "rnp_key_export"
string(16) "rnp_key_get_info"
string(13) "rnp_list_keys"
string(14) "rnp_locate_key"
string(14) "rnp_op_encrypt"
string(19) "rnp_op_generate_key"
string(21) "rnp_op_sign_cleartext"
string(20) "rnp_op_sign_detached"
string(11) "rnp_op_sign"
string(22) "rnp_op_verify_detached"
string(13) "rnp_op_verify"
string(22) "rnp_supported_features"
string(36) "sodium_crypto_aead_aegis128l_decrypt"
string(35) "sodium_crypto_aead_aegis256_decrypt"
string(19) "ssh2_forward_accept"
string(19) "ssh2_forward_listen"
string(21) "odbc_procedurecolumns"
string(15) "odbc_procedures"

@staabm
Copy link
Collaborator

staabm commented Dec 2, 2024

nice thank you. maybe someone could just open a PR at the PHPStan end to add the signatures?

the "rnp" extensions seem experimentell, therefore I would be fine in case we could skip it just by hardcoding it somewhere

@icanhazstring
Copy link

Will create a Issue in PHPStan and link it.
Lets see what ondrej seems to do :)

@icanhazstring
Copy link

icanhazstring commented Dec 2, 2024

So here is the official answer
phpstan/phpstan#12169 (comment)

I guess in the meantime, we can "just ignore" all these functions, to at least keep the functionality and maybe "rework" how this library is writing all the generated files? Don't know :)

@staabm
Copy link
Collaborator

staabm commented Dec 2, 2024

ok, then lets have a dedicated ignore list somewhere in the generator
(and it would be great we would get a better error message, when we run into the same problem in the future with a new function)

@icanhazstring
Copy link

icanhazstring commented Dec 2, 2024

ok, then lets have a dedicated ignore list somewhere in the generator (and it would be great we would get a better error message, when we run into the same problem in the future with a new function)

I guess this file should be suffice?
https://github.com/thecodingmachine/safe/blob/master/generator/config/ignoredFunctions.php

Let me prepare a PR with the ignored file for now.
And update the error message a little bit better.

@staabm
Copy link
Collaborator

staabm commented Dec 2, 2024

I guess this file should be suffice?

I guess this could work. I am not yet comfortable enough with the codebase to say for sure

@icanhazstring
Copy link

Will check 🙂

@staabm
Copy link
Collaborator

staabm commented Dec 2, 2024

@icanhazstring just fyi, please see the comments in #481 (comment)

the problems you are trying to solve, maybe be already fixed and in the process of beeing submitted.
thanks for the work you put into it though.

@shish
Copy link
Collaborator

shish commented Dec 2, 2024

I thiiink once #486 #487 and #488 are merged, we should have working unit tests and working auto-generation, and then the github cron-job should keep us up to date automatically from there \o/ (though it still needs a human to accept the bot-generated PR)

@github-actions github-actions bot force-pushed the create-pull-request/regenerate-files branch from 7c9d090 to 38cb235 Compare December 2, 2024 18:28
@staabm
Copy link
Collaborator

staabm commented Dec 2, 2024

Manually triggered the action. We got a non-empty PR 🚀

🤠

@github-actions github-actions bot force-pushed the create-pull-request/regenerate-files branch from 38cb235 to 5f377c1 Compare December 3, 2024 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants