Skip to content
This repository has been archived by the owner on Dec 18, 2022. It is now read-only.

Vamp SDK filename fixes #584

Merged
merged 1 commit into from
Sep 12, 2021
Merged

Vamp SDK filename fixes #584

merged 1 commit into from
Sep 12, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Sep 5, 2021

Resolves: #526

If the Windows build works, I'll merge the vamp_sdk_filename branch of the vcpkg repo to the tenacity branch and make a PR for upstream vcpkg.

Checklist
  • I have signed off my commits using -s or Signed-off-by* (See: Contributing § DCO)
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code*
  • I made sure the title of the PR reflects the core meaning of the issue you are solving*
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

* indicates required

@Be-ing Be-ing requested a review from emabrey September 5, 2021 00:58
@Be-ing Be-ing mentioned this pull request Sep 5, 2021
Closed
5 tasks
@Be-ing Be-ing marked this pull request as draft September 5, 2021 00:59
@emabrey
Copy link
Member

emabrey commented Sep 5, 2021

Your commit title starts with a lowercase letter.

@emabrey
Copy link
Member

emabrey commented Sep 5, 2021 •
edited
Loading

You need to pull the commits from my YASM/NASM fixes to vcpkg on to your branch to prevent regression. (Just NASM actually, the YASM fix hasn't been merged yet).

tenacityteam/vcpkg@149fdbc

@emabrey
Copy link
Member

emabrey commented Sep 5, 2021

I tried this on my system locally with a Debug build, which is what my fix already successfully fixes, and I got this error:

LINK : fatal error LNK1104: cannot open file '..\vcpkg_installed\x64-windows\lib\vamp-hostsdk.lib' [C:\Users\emily\Documents\GitHub\tenacityteam-tenacity\build\src\Tenacity.vcxproj]

emabrey
emabrey suggested changes Sep 5, 2021
View reviewed changes
Copy link
Member

@emabrey emabrey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This needs to include my NASM workaround fixes for the vcpkg issue I addressed a few days ago. Additionally, this doesn't currently work and gives an error at link time.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 5, 2021

got this error

vcpkg might not have rebuilt the package because I forgot to bump the port version before.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 5, 2021

Debug build on GitHub Actions worked:

-- Found VampHostSDK: D:/a/tenacity/tenacity/build/vcpkg_installed/x64-windows/debug/lib/VampHostSDK.lib  
-- VAMP plugin hosting enabled.

Please test the current state of this branch locally.

@emabrey
Copy link
Member

emabrey commented Sep 5, 2021

It worked. Shouldn't you be fixing this upstream though and not doing it as an overlay?

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 5, 2021

I am using the overlay to confirm it works before making an upstream pull request. It typically takes about a week for a pull request to be merged upstream. We don't need to wait for that before fixing it for ourselves. Once it is merged upstream we can remove the vamp-sdk package from our overlay.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 5, 2021

Upstream vcpkg PR: microsoft/vcpkg#20002

@Be-ing Be-ing marked this pull request as ready for review September 5, 2021 16:34
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 5, 2021

I cherry-picked the vamp-sdk fix and the NASM workaround to the tenacity branch of the vcpkg repo and switched the submodule back to the tenacity branch.

nbsp
nbsp suggested changes Sep 6, 2021
View reviewed changes
Copy link
Contributor

@nbsp nbsp left a comment

Choose a reason for hiding this comment

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

Can't directly review because it's in a submodule, but
vcpkg/overlay/ports/vamp-sdk/vcpkg.json:

  "description": "Library for VAMP plugins",

VAMP => Vamp

emabrey
emabrey previously approved these changes Sep 6, 2021
View reviewed changes
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 6, 2021

@solfisher that is extremely minor and inconsequential to Tenacity, but okay, fixed.

emabrey
emabrey previously approved these changes Sep 6, 2021
View reviewed changes
nbsp
nbsp previously approved these changes Sep 6, 2021
View reviewed changes
@emabrey
Copy link
Member

emabrey commented Sep 7, 2021 •
edited
Loading

@Be-ing Can you pull in upstream changes/force push to this branch before I merge this? I was about to do the merge but I noticed that the vcpkg commit is a reversion and I just want to be sure not to break master. Once you do that I will merge it since it's already approved.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 8, 2021

Huh? What reversion are you referring to?

@Be-ing Be-ing dismissed stale reviews from nbsp and emabrey via f2c25fa September 8, 2021 14:58
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 8, 2021

My pull request was merged upstream so I removed vamp-sdk from the overlay and merged the latest vcpkg upstream.

@Be-ing
Fix mixup of vamp-sdk release and debug libraries
51c413b 
There were two problems:
1. The vamp-sdk vcpkg port added a `d` suffix for debug builds
that upstream did not.
2. The vamp-sdk vcpkg port used the same file name for libraries
on every OS but that is not what upstream does.

Signed-off-by: Be <be@mixxx.org>
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 11, 2021

Merge?

@emabrey emabrey merged commit 01f22e8 into tenacityteam:master Sep 12, 2021
@Be-ing Be-ing deleted the vamp_sdk_filename branch September 12, 2021 23:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@n0toose n0toose n0toose approved these changes

@emabrey emabrey emabrey left review comments

@nbsp nbsp nbsp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows build is broken by missing vamp-hostsdk.dll
4 participants
@Be-ing @emabrey @n0toose @nbsp