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

[liblsl] Update library to 1.13.0-b11 #7906

Merged
merged 3 commits into from
Aug 27, 2019
Merged

Conversation

ehsan-mohammadi
Copy link
Contributor

@ehsan-mohammadi ehsan-mohammadi commented Aug 26, 2019

  • Update liblsl library to 1.13.0-b11 version.
  • Add fix-install.patch patch file to remove .exe files and avoid build failing.

@ehsan-mohammadi ehsan-mohammadi marked this pull request as ready for review August 26, 2019 13:56
@cbezault cbezault merged commit 8cf2067 into microsoft:master Aug 27, 2019
@ehsan-mohammadi ehsan-mohammadi deleted the dev0 branch August 27, 2019 21:37
@cenit
Copy link
Contributor

cenit commented Aug 27, 2019

Add fix-install.patch patch file to remove .exe files and avoid build failing.

@cbezault @ehsan-mohammadi lslver is an executable. Why has it been arbitrarily transformed into a static library and why the change has been approved with a patch?

@ehsan-mohammadi
Copy link
Contributor Author

ehsan-mohammadi commented Aug 28, 2019

@cenit Thanks for reviewing this PR! When we try to install liblsl, the lslver.exe be made and it avoids installing liblsl successfully. Do you have any suggestion to solve this problem?

@cenit
Copy link
Contributor

cenit commented Aug 28, 2019

liblsl.exe should have been moved to the tools folder or removed, not transformed into a "fake" library.
(Luckily) There is plenty of people now here in this repo marked as "internal" in vcpkg but still some basic checks when accepting PRs are still missing from the table. It's not a race to "do work", it should be a race to do "good work".

@ehsan-mohammadi
Copy link
Contributor Author

ehsan-mohammadi commented Aug 28, 2019

@cenit I know, and thanks a lot for helping me. I'm new in vcpkg and need to learn more. I'll fix this issue.

@cenit
Copy link
Contributor

cenit commented Aug 28, 2019

no problem, it's not against you of course.
I like your good attitude and the fact that you're trying your best, and doing a lot of work. I can't find a common topic in your PRs so (guessing) maybe you just want to contribute to this project without much in return, and this is wonderful.
Telling you to improve the "product" (the PR) should be others' work, in order to make your efforts even more fruitful, so I am also sorry if you see me rude. I do it only because I saw too many times ports broken just because PRs have been merged too easily. And since you are very active, the probability of you touching any port on which I depend grows a lot.

On your side, since it seems that the green mark is usually enough to grant a merge, please try to do your best to improve port and not just modify references to a newer release: check every patch (new and already existing) to be sure that works and is valid, check tools if properly installed with a feature, verify if any feature is missing or non-working (CI, for ports without dependencies that might trigger extra ones, just checks default features, unfortunately), try to use the library in a dependent project. Just trying to link to it with cmake would be enough and very easy to do, even without any project. It wouldn't make you sure that the library fulfills consumers' expectations, but at least you are verifying that the toolchain in properly in place

@ehsan-mohammadi
Copy link
Contributor Author

@cenit Thanks a lot. I will warmly welcome your comments.

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.

3 participants