-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[libclipboard] new port #34764
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
[libclipboard] new port #34764
Conversation
|
This repository is licensed under the MIT license. What reason is there to completely reassign my copyright to Microsoft? By contributing, I am licensing my changes under the MIT license. Is that not enough? |
(which is Linux too, but whatever)
Wasn't intended to, so I won't bother trying to fix it.
|
I can squash my crazy number of commits if need be. |
|
dg0yt
left a comment
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.
The patch seem more invasive than necessary.
|
@microsoft-github-policy-service agree I morally disagree with the CLA, but it's clear to me that unless I give Microsoft the right to use my work without so much as a mention of my existence, no movement will happen. |
|
The usage test passed on |
dg0yt
left a comment
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.
I still think the patch is much too invasive, not minimal (as required by guidelines).
I also dislike the removal of the official pkg-config files.
An incomplete set of hints follows.
Disclaimer: I'm only a community member,
| -elseif(LIBCLIPBOARD_BUILD_X11) | ||
| - include(FindPkgConfig REQUIRED) | ||
| - pkg_check_modules(X11 xcb REQUIRED) | ||
| - find_package(Threads REQUIRED) | ||
| - | ||
| - include_directories(${X11_INCLUDE_DIRS}) | ||
| - link_directories(${X11_LIBRARY_DIRS}) | ||
| - set(LIBCLIPBOARD_PRIVATE_LIBS ${LIBCLIPBOARD_PRIVATE_LIBS} ${X11_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) |
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.
Why is this removed?
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.
I'll admit, my brain went "not modern CMake, kill!". Perhaps I'll fork the repository to bring the CMake stuff up to standard, since it's not been updated in three years anyway.
| set(SOURCE | ||
| src/clipboard_win32.c | ||
| src/clipboard_x11.c | ||
| - src/clipboard_cocoa.c |
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.
| - src/clipboard_cocoa.c |
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.
This file will not build in the slightest (even with definition guards) without an Objective-C compiler, which we don't want to assume the presence of unless Cocoa is requested (read: on a Mac).
| +if(LIBCLIPBOARD_BUILD_COCOA) | ||
| +list(APPEND SOURCE src/clipboard_cocoa.c) | ||
| endif() |
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.
| +if(LIBCLIPBOARD_BUILD_COCOA) | |
| +list(APPEND SOURCE src/clipboard_cocoa.c) | |
| endif() | |
| endif() |
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.
| endif() | ||
|
|
||
| -# Testing mode? | ||
| -if (EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/third_party/googletest/googletest) |
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.
It is easy to ensure that the file doesn't exist, so we don't need to remove all these lines.
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.
Fair enough.
|
I agree with @dg0yt regarding the patch. I like the modernization, but we should really try to follow upstream's lead whenever possible and keep patch sizes down to a minimum. I'll place the PR on draft for now. |
Our lawyers told us that this is the reason: Microsoft asks that all contributors agree to a contributor license agreement (CLA). CLAs are generally common and accepted in most open source software projects. We all want Microsoft's open source projects to be as widely used and distributed as possible. We also want its users to be confident about the origins and continuing existence of the code. The CLA help us achieve that goal by ensuring that we have the agreement of our contributors to use their work, whether it be code, or documentation. Microsoft to distribute your code without restriction. It doesn't require you to assign to us any copyright you have, the ownership of the copyright remains with you. You cannot withdraw permission for use of the contribution at a later date. We are generally seeking originally authored code and documentation as contributions. Should you wish to submit materials that are not your original work, you may submit them separately to the Project in accordance with the terms of the CLA. |
|
Thanks for your contribution. If you're ready, please revert to "ready for review". If not, please continue working on this PR. |
|
Closing this PR since it seems that no progress is being made. Please ping us to reopen if work is still being done. |
find_packagecalls are REQUIRED, are satisfied byvcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxxvcpkg.jsonmatches what upstream says.vcpkg.jsonmatches what upstream says../vcpkg x-add-version --alland committing the result.