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.

Sign up for GitHub

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

Jump to bottom

[libuvc] added #14749

Merged
merged 1 commit into from
Feb 9, 2021
Merged

[libuvc] added #14749

merged 1 commit into from
Feb 9, 2021

Conversation

fran6co
Copy link
Contributor

@fran6co fran6co commented Nov 24, 2020

No description provided.

@fran6co fran6co mentioned this pull request Nov 24, 2020
Merged
@NancyLi1013 NancyLi1013 self-assigned this Nov 25, 2020
@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 25, 2020
NancyLi1013
NancyLi1013 suggested changes Nov 25, 2020
View reviewed changes
ports/libuvc/portfile.cmake Outdated Show resolved Hide resolved
ports/libuvc/portfile.cmake Outdated Show resolved Hide resolved
ports/libuvc/portfile.cmake Outdated Show resolved Hide resolved
ports/libuvc/CONTROL Outdated Show resolved Hide resolved
ports/libuvc/portfile.cmake Show resolved Hide resolved
@fran6co fran6co force-pushed the libuvc branch 2 times, most recently from 3468c61 to 2064716 Compare November 25, 2020 11:34
@NancyLi1013
Copy link
Contributor

The failures on windows:

CMake Error:
  Running

   'D:/downloads/tools/ninja/1.10.1-windows/ninja.exe' '-C' 'D:/buildtrees/libuvc/x86-windows-rel' '-t' 'cleandead'

  failed with:

   ninja: error: build.ninja:229: multiple rules generate uvc.lib [-w dupbuild=err]

Needs to judge the value of CMAKE_BUILD_TARGET to choose which targets should be built.

Also the file #include <sys/time.h> included in include/libuvc/libuvc.h doesn't exist on Windows platform, which also needs to fix.

@fran6co
Copy link
Contributor Author

fran6co commented Nov 26, 2020

@NancyLi1013 is it possible to mark this package as linux only? According to libuvc/libuvc#12 it seems that Windows support is a bit uncomplete. The main reason I had to add this package is for getting azure-kinect-sensor-sdk to work on linux. I'm guessing that package already works on Windows without libuvc, right? So I could just make this dependency linux only

@NancyLi1013
Copy link
Contributor

@NancyLi1013 is it possible to mark this package as linux only? According to libuvc/libuvc#12 it seems that Windows support is a bit uncomplete. The main reason I had to add this package is for getting azure-kinect-sensor-sdk to work on linux. I'm guessing that package already works on Windows without libuvc, right? So I could just make this dependency linux only

Sorry for the delay. Sure, we can only support Linux for linuvc. It might also support osx. You can try this.

For the dependency, you can set it as libuvc (!windows) or libuvc (linux). Please help confirm this point.

@NancyLi1013
Copy link
Contributor

@fran6co

Is work still being done for this PR?

@fran6co
Copy link
Contributor Author

fran6co commented Jan 14, 2021

@NancyLi1013 not sure what else should I do for this, this package will not work for Windows without upstream changes

@NancyLi1013
Copy link
Contributor

You can add Supports: ! windows in CONTROL file until upstream supports it you can remove this.

@NancyLi1013
Copy link
Contributor

@fran6co

Could you please address the review suggestion? Please let me know if you have any questions about this. Thanks.

@fran6co
Copy link
Contributor Author

fran6co commented Feb 8, 2021

@NancyLi1013 just added the Supports option

@fran6co fran6co force-pushed the libuvc branch 2 times, most recently from 5ce8e51 to d3a73d3 Compare February 8, 2021 11:52
@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Feb 9, 2021
@NancyLi1013
Copy link
Contributor

LGTM, thanks for your PR @fran6co.

@JackBoosY
Copy link
Contributor

Ping @ras0219-msft for review and merge this PR since it block another PR.

@ras0219-msft ras0219-msft merged commit 5957dcb into microsoft:master Feb 9, 2021
@ras0219-msft
Copy link
Contributor

Thanks for the new port!

@fran6co fran6co deleted the libuvc branch February 9, 2021 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fran6co @NancyLi1013 @JackBoosY @ras0219-msft