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 shiboken includes to fix Windows build. #938

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

acolwell
Copy link
Collaborator

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

PR Description

What type of PR is this? (Check one of the boxes below)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
    • I have updated the documentation accordingly

What does this pull request do?

Pyside2 and a few other python related packages were updated on MSYS2 in the last week or so which caused the Windows Installer builds to start failing. This change modifies how include paths are specified on the command-line. It appears that shiboken no longer handles colon separated path lists so each include is now specified with its own -I option.

Have you tested your changes (if applicable)? If so, how?

Yes. I verified that these changes fix the installer build locally and via the GitHub action (https://github.com/acolwell/Natron/actions/runs/7514747615)

Pyside2 and a few other python related packages were updated
on MSYS2 in the last week or so which caused the Windows Installer
builds to start failing. This change modifies how include paths
are specified on the command-line. It appears that shiboken no
longer handles colon separated path lists so each include is now
specified with its own -I option.
@devernay
Copy link
Member

Does that fail because one or more of the path components contain spaces? In which case double quotes around the variable expansion in the shiboken2 call could also fix it

@devernay devernay self-requested a review January 18, 2024 02:53
@acolwell
Copy link
Collaborator Author

Does that fail because one or more of the path components contain spaces? In which case double quotes around the variable expansion in the shiboken2 call could also fix it

It fails because shiboken2 uses a different token to separate paths. On Linux (and I suspect Mac) it uses ':' and on Windows it uses ';'. You can see this by looking at the 'shiboken2 --help' text.

I've also just updated the typesystem path args since the help text reports the same platform difference. I'm not sure why ':' seemed to still work for that argument, but I figure changing it now should help avoid future breakages if they ever change that behavior.

@acolwell acolwell merged commit 91fdcbc into NatronGitHub:RB-2.5 Jan 18, 2024
3 checks passed
@acolwell acolwell deleted the fix-shiboken-commandlines branch January 18, 2024 18:24
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.

2 participants