FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: wheel for windows by Tieqiong · Pull Request #111 · diffpy/diffpy.pdffit2 · GitHub

  1. diffpy
  2. diffpy.pdffit2

Repository navigation

fix: wheel for windows - #111

Merged
sbillinge merged 4 commits into
diffpy:mainfrom
Tieqiong:winwheel
Jan 4, 2025
ConversationCommits (4)ChecksFiles changed
Merged

fix: wheel for windows #111
sbillinge merged 4 commits into
diffpy:mainfrom
Tieqiong:winwheel

Conversation

Tieqiong commented Jan 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

@sbillinge This should fix diffpy/diffpy.pdfgui#238 and diffpy/diffpy.pdfgui#240 after another pypi release. Before merging this the workflow file still need to be changed. The one attached here is just for showing it works.

@bobleesj Can you help merging this workflow file into the release-script central workflow? Thanks!

These are the local test of the wheel produced here

Windows with python3.12:

Linux with python3.12:

Mac Arm with python3.12:

github-actions Bot commented Jan 2, 2025

Copy link
Copy Markdown

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.

bobleesj commented Jan 2, 2025

Copy link
Copy Markdown
Contributor

Awesome Tieqiong. Yes, once merged, I will do copy and paste.

bobleesj mentioned this pull request Jan 2, 2025
Closed
Tieqiong mentioned this pull request Jan 2, 2025
Merged

Tieqiong commented Jan 2, 2025

Copy link
Copy Markdown
Contributor Author

@bobleesj Maybe it's better to copy the workflow before this get merged, because I don't want the dummy workflow to be merged. I will delete it after Simon review it and before merge.

bobleesj commented Jan 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

👍 understood.

Once @sbillinge gives it a quick review, we can try rc7 release of pdffit then? @Tieqiong

sbillinge reviewed Jan 2, 2025
View reviewed changes

sbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @Tieqiong this is very exciting! I left a couple of housework kind of comments, but when they are addressed, this look good to go.

Comment thread .github/workflows/wheel.yml
name: Wheel builder

on:
pull_request:

sbillinge Jan 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

shall we do just pull_request and push into main? I think that is our standard on our other workflows

Tieqiong Jan 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@sbillinge I'll delete this workflow file before merging. This is just for showing the changes work here, and it would be easier for @bobleesj to copy to the central release-script. So details shouldn't matter too much.

bobleesj Jan 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Tieqiong scikit-package/release-scripts#123

I copied and saved in the issue. pls confirm if that's all good.

Tieqiong Jan 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@bobleesj please see comment there

sbillinge Jan 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As a general rule I like taking care of details when we think of them, otherwise technical debt tends to just start piling up. But if this is going to be completely gone through again later and everything fixed, i htink it is ok to let things slide here.

Comment thread setup.py Outdated
class CustomBuildExt(build_ext):
def run(self):
super().run()
gsl_path = os.environ.get("GSL_PATH") or os.path.join(os.environ.get("CONDA_PREFIX", ""), "Library")

sbillinge Jan 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would prefer if we could use pathlib.Path() as much as possible for these operations, for easier future maintainability.

Tieqiong Jan 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@sbillinge I changed most of the os.path to pathlib. Please check, thanks

codecov Bot commented Jan 4, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.96%. Comparing base (154c87a) to head (7e12beb).
Report is 8 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #111   +/-   ##
=======================================
  Coverage   98.96%   98.96%           
=======================================
  Files           6        6           
  Lines        1261     1261           
=======================================
  Hits         1248     1248           
  Misses         13       13

sbillinge merged commit 46c5082 into diffpy:main Jan 4, 2025
Tieqiong deleted the winwheel branch January 12, 2025 02:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sbillinge sbillinge left review comments
+1 more reviewer
bobleesj bobleesj left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Windows Python 3.11, 3.12 matrix CI fail due to ImportError: DLL load failed while importing pdffit2

3 participants

Back | FazBrowse Home | New Git URL