-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[portmidi] Updated portmidi package to use alternative code source #12046
Conversation
Thanks for the feedback. I've made the changes suggested except for the empty options one, this seems to be required by the package. |
@stekyne |
Portmidi is crossplatform so it should work yes. I haven't tested these specific package changes on OSX/Linux though. As far as I can tell, they're fairly platform agnostic? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved changes requested
Since I noticed that You can enter the above link and search |
Ok I'll need to take a look. It's missing Java but can't see how to disable that with a single flag. I'll get back to this shortly. |
Could we use https://sourceforge.net/code-snapshots/svn/p/po/portmedia/code/portmedia-code-r234.zip instead, potentially with a small patch checked into this repo? We strongly prefer not pulling unofficial forks if possible. |
@ras0219-msft sure I can do that. Your link no longer works though. I see you can download a snapshot of the repo on the site directly but it seems to be an ephemeral thing? Is that used by other packages? |
@stekyne |
@NancyLi1013 I will but I need some clarification on my last question. Is it ok to move forward with using a non-official fork of the source or do I need to use an official zip? The link provided doesn't work after a time, so I'm not sure it's viable to use that method. |
@stekyne |
@stekyne |
c32859c
to
dda533c
Compare
@NancyLi1013 Sorry for the delay. I managed (with help) to get the original author to release a new zip of the source. It's been updated and I think the CI is passing. Can this be merged? |
@stekyne I noticed that it is set as fail on x64-linux and x64-osx in ci.baseline.txt. |
@NancyLi1013 can you point me to the CI failure? I saw all green ticks on the details panel. |
I mean the related triplets were set as fail in ci.baseline.txt. https://github.com/microsoft/vcpkg/blob/master/scripts/ci.baseline.txt#L1360-L1361
I also checked the results on CI and they skipped. I tried to install it on my machine and it didn't work. So could you help if it doesn't support linux and osx? If not, we'd better add |
@stekyne Can you confirm that you're happy with my changes? |
Thanks for taking a look. I've been trying to get it building for osx and linux in the mean time since updating. The cmake in portmidi is a bit inflexible so im trying to find out the right solution. Either fix it upstream or make a quick patch. Basically just need to allow building without jni support. |
Co-authored-by: nicole mazzuca <[email protected]>
Thanks for your contribution! |
…icrosoft#12046) Co-authored-by: Billy Robert O'Neal III <[email protected]> Co-authored-by: nicole mazzuca <[email protected]>
Describe the pull request
Updates the portmidi package to use the latest source. As discussed in #11974. Fixes #11974
What does your PR fix? Fixes #
The previous version of the package was using a zip release from sourceforge which is 10 years old. It doesn't actually work with x64 Windows. There are commits after this release in the sourceforge repo that resolve the issue. I have migrated the sourceforge repo to git (under the Csound org github account). I'll update this going forward.
Which triplets are supported/not supported? Have you updated the CI baseline?
x86/x64 windows are now supported
Does your PR follow the maintainer guide?