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

Removed Python imp dependency #1460

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

tazmaniax
Copy link
Collaborator

@tazmaniax tazmaniax commented Dec 10, 2023

Fixes issue #1457

@xabolcs
Copy link
Contributor

xabolcs commented Dec 10, 2023

Would you mind add a link to the issue in the commit or at least in the PR title/description?

@davidcostanzo
Copy link
Contributor

The Check failures looks like another problem with CI that was hiding behind the "imp" problem. I applied this PR to a private fork and was able to run tests.py without problem on one machine. On another machine, it hangs in the same place as it did for this PR:

RUN  ['/home/runner/work/play1/play1/samples-and-tests/i-am-a-developer/i-am-testing-ssl-config-here/../../../play', 'run', '/home/runner/work/play1/play1/samples-and-tests/i-am-a-developer/i-am-testing-ssl-config-here/sslconfigapp']

The request to https://localhost:9001 hangs without Play ever handling the request. While it's hanging, I can use both google-chrome and curl to request https://localhost:9001 (after instructing them to allow self-signed certs). Is there a way that this PR can be accepted and the problem with tests.py hanging be tracked as a separate issue? Or should this PR encompass everything needed to get the CI tests to run again?

davidcostanzo pushed a commit to davidcostanzo/play1 that referenced this pull request Feb 2, 2024
…chines.

This may fix Play1's broken CI checks on GitHub.

This is based on playframework#1460 by tazmaniax.
That PR removed the dependency on imp.

Other updates:
- mechanize.browser locks up when requesting https URLs
- invoking Response.getStream() multiple times on different Responses from
  the same URL results in a SocketException.
- assert_ no longer exists (assertTrue looks like a good replacement)
- on some machines "python" links to python2, not python3.

All of these changes are lumped into a single PR because, if the changes
were submitted separately, none of them would pass the CI Checks.
@davidcostanzo
Copy link
Contributor

@tazmaniax, I applied your PR to a private branch and worked around the problem that caused infinite hang. Then I had to fix a few other problems. On my machines "ant test" now works. I don't know Git well enough to have applied your PR to my branch in a way that GitHub knows it came from you, so if I submit a PR, you won't get credit for fixing the CI. Do you want to work my other changes into your PR or should I submit our combined changes as my own PR?

@tazmaniax
Copy link
Collaborator Author

@davidcostanzo amazing! I'm happy to work your changes into this PR even just to understand what the changes were. Thanks!

@davidcostanzo
Copy link
Contributor

I'm happy to work your changes into this PR even just to understand what the changes were

@tazmaniax, are you able to see the changes I made at davidcostanzo@faf6ad5? I don't know GitHub well so I don't know what's public and what's not.

Most of the changes are small tweaks to python or ant code to be compatible with python3. The change that I'm least confident in is the one to WSTest.java, because I worked around a change in behavior instead of fixing it. I suspect that connections to the same URL are pooled and by calling getStream() multiple times, it puts something lower down into a bad state. Personally, I'd rather getStream() throw an IllegalStateException if invoked multiple times rather than return a stream of zero bytes.

I submitted davidcostanzo#2 from my fork's branch to my forks master as a way to run the PR checks. It looks like they passed, but the "windows-latest" check shows errors in the log like the one below that make me think its python isn't set up properly. I don't know how it could have passed with these errors.

     [exec] RUN  ['D:\\a\\play1\\play1\\samples-and-tests\\i-am-a-developer\\i-am-testing-ssl-config-here\\../../../play.bat', 'new', 'D:\\a\\play1\\play1\\samples-and-tests\\i-am-a-developer\\i-am-testing-ssl-config-here/sslconfigapp', '--name=SSLCONFIGAPP']
     [exec] RUNNING  5980
     [exec] .Traceback (most recent call last):
     [exec]   File "D:\a\play1\play1\framework\pym\play\cmdloader.py", line 28, in load_core
     [exec]     mod = load_python_module(module_name, module_path)
     [exec]           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     [exec]   File "D:\a\play1\play1\framework\pym\play\cmdloader.py", line 69, in load_python_module
     [exec]     spec.loader.exec_module(mod)
     [exec]   File "<frozen importlib._bootstrap_external>", line 994, in exec_module
     [exec]   File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
     [exec]   File "D:\a\play1\play1\framework\pym\play\commands\daemon.py", line 14, in <module>
     [exec]     import win32pdh, win32pdhutil
     [exec] ModuleNotFoundError: No module named 'win32pdh'

I don't have a Windows machine that I can experiment on, but I saw in another thread, someone ask "who cares about windows?", so maybe it doesn't matter.

By the way, I'm new to GitHub and I hope that the way I'm doing things hasn't messed stuff up for you.

@xael-fry xael-fry merged commit 76ae694 into playframework:master Feb 27, 2024
2 of 5 checks passed
@tazmaniax
Copy link
Collaborator Author

@xael-fry thanks for getting this out the door. Much appreciated!

@xael-fry xael-fry self-requested a review February 27, 2024 09:32
@xael-fry xael-fry added this to the 1.8.0 milestone Feb 27, 2024
@tazmaniax
Copy link
Collaborator Author

@davidcostanzo apologies, I didn't get around to incorporating your changes before this was merged. I suggested you create a new pull request based on the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants