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

Update coolprop to 6.6.0 #4397

Merged
merged 9 commits into from Jan 28, 2024
Merged

Update coolprop to 6.6.0 #4397

merged 9 commits into from Jan 28, 2024

Conversation

dvd101x
Copy link
Contributor

@dvd101x dvd101x commented Jan 20, 2024

Updated the coolprop version and changed source according to #4028 (comment)

Description

  • Updated coolprop version to 6.6.0
  • Changed source from sourceforge to pypi

Checklists

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

Changed the coolprop version and source according to pyodide#4028 (comment)
@hoodmane
Copy link
Member

Thanks @dvd101x!

@dvd101x dvd101x marked this pull request as ready for review January 20, 2024 21:31
@hoodmane
Copy link
Member

You'll need to merge main to get #4396 which fixes the build.

@dvd101x
Copy link
Contributor Author

dvd101x commented Jan 20, 2024

Thanks! I did the merge.

Attempting to fix error of no file or directory "source" the new source is called "CoolProp-6.6.0"
@dvd101x
Copy link
Contributor Author

dvd101x commented Jan 21, 2024

Hi Hood,

Thanks for the help with the merge suggestion.

Sadly I might need help on this one. I'm really new to this. Could you give me some pointers on my mistake?

@hoodmane
Copy link
Member

Error building sourmash.

Not sure, looks unrelated to you since sourmash doesn't depend on coolprop. I restarted the CI, hopefully it was a flake. If not a flake and there was a change outside of this repo that caused sourmash build to persistently fail, then you could try disabling sourmash to see what happens with coolprop.

@hoodmane
Copy link
Member

Okay on rerun we get an actual failure:

FileNotFoundError: [Errno 2] No such file or directory:
'/root/repo/packages/coolprop/build/coolprop-6.6.0/wrappers/Python'

I guess when I get a chance I'll have to try to build it locally and see what happens. Did you build it locally and succeed or are you unable to do local builds?

@hoodmane
Copy link
Member

hoodmane commented Jan 22, 2024

I can reproduce the failure locally. It seems that Coolprop does some file rearrangement on its sdist when you run setup.py and somehow our build system isn't reproducing it properly. Maybe one of the Coolprop maintainers (cc @ibell, @jowr) could give us some guidance, or maybe comparing the build logs between building a native wheel and a Pyodide wheel will show the steps that we're missing.

@ryanking13
Copy link
Member

Could you try removing these two line and see what happens?

- - extras/setup.py
- ./setup.py

The coolprop archive from Sourceforge and from PyPI have different structures. I guess the line above was to workaround for the Sourceforge archive so you might not need it for PyPI archive.

@dvd101x
Copy link
Contributor Author

dvd101x commented Jan 22, 2024

I tried removing the lines for setup.py but it seems to fail quicker.

I don't have a setup ready to reproduce locally, but there is also a newer version in sourceforge that I could try instead of the one from GitHub.

@hoodmane
Copy link
Member

hoodmane commented Jan 22, 2024

Most recent failure was in scikit-image with:

http.client.IncompleteRead: IncompleteRead(11534336 bytes read, 11186083 more

Which looks like another flake regarding some sort of failed http request unrelated to this PR. I don't know why we're getting so many failed requests on this PR, it's not usually very common.

Anyways I reran it.

@ryanking13
Copy link
Member

The build was successful, but there are test failures. Could you check and fix the test failure?

@dvd101x
Copy link
Contributor Author

dvd101x commented Jan 26, 2024

It seems it was missing some sources as mentioned in here, had to revert back to sourceforge
CoolProp/CoolProp#2305 (comment)

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ryanking13 ryanking13 merged commit 99f6cf4 into pyodide:main Jan 28, 2024
41 checks passed
ryanking13 pushed a commit to ryanking13/pyodide that referenced this pull request Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants