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

Add C package xsel recipe #26333

Merged
merged 14 commits into from
Jun 1, 2024
Merged

Add C package xsel recipe #26333

merged 14 commits into from
Jun 1, 2024

Conversation

ehfd
Copy link
Member

@ehfd ehfd commented May 11, 2024 •
edited

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/xsel) and found it was in an excellent condition.

@ehfd ehfd marked this pull request as ready for review May 11, 2024 18:30
@ehfd
Copy link
Member Author

ehfd commented May 11, 2024

@conda-forge/help-c-cpp Ready for review.

This package interfaces with the X11 clipboard API.

Since this is not an official X.Org package, keeping Unix-only for now.

@ehfd
Copy link
Member Author

ehfd commented May 11, 2024

@hmaarrfk
I have seven X11-related packages waiting to be merged. Could you possibly help out?

@ehfd
Copy link
Member Author

ehfd commented May 26, 2024

@xhochy Very thankful if these could be processed in timely fashion:

#26163
#26165
#26166
#26242
#26254
#26256
#26333

I am waiting for this to do a release on my own project.

hmaarrfk
hmaarrfk reviewed May 29, 2024
View reviewed changes
recipes/xsel/meta.yaml Outdated
- automake # [unix]
- gettext # [unix]
- libtool # [unix]
- xorg-libxt
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be in the host section?

Copy link
Member Author

@ehfd ehfd May 29, 2024 •
edited

Choose a reason for hiding this comment

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

They're build/make dependencies as per Homebrew and Arch. And also it's not linked anywhere and probably just needs the headers.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://formulae.brew.sh/formula/xsel

https://archlinux.org/packages/extra/x86_64/xsel/

Copy link
Member

Choose a reason for hiding this comment

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

Even if it just needs the headers, it should go into host. Different tools/ecosystems don't necessarily agree on all details of naming/semantics, but in conda-forge, the build environment is restricted purely to things that need to be executed during the build (like a compiler); everything you want to compile against, whether shared, static or header-only, needs to go into host.

Copy link
Member Author

@ehfd ehfd May 30, 2024 •
edited

Choose a reason for hiding this comment

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

Moved to headers and instead put in ignore_run_exports then.

Copy link
Member Author

Choose a reason for hiding this comment

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

@h-vetinari

@ehfd ehfd requested a review from hmaarrfk May 29, 2024 08:35
@ehfd
Copy link
Member Author

ehfd commented May 30, 2024

@xhochy Very thankful if these could be processed in timely fashion:

#26163 #26165 #26166 #26242 #26254 #26256 #26333

I am waiting for this to do a release on my own project.

@h-vetinari Since you're in the core team, could you help this out? It's been over a month.

@ehfd
Copy link
Member Author

ehfd commented May 30, 2024

This is an optional dependency of (indirectly) pandas and (directly) pyperclip.

@ehfd ehfd requested a review from h-vetinari May 30, 2024 12:34
h-vetinari
h-vetinari reviewed May 31, 2024
View reviewed changes
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Basically LGTM, just some small details to fix up

recipes/xsel/build.sh Outdated Show resolved Hide resolved
recipes/xsel/meta.yaml Outdated Show resolved Hide resolved
recipes/xsel/meta.yaml Outdated Show resolved Hide resolved
@ehfd ehfd requested a review from h-vetinari May 31, 2024 09:47
@ehfd ehfd requested a review from h-vetinari June 1, 2024 06:54
h-vetinari
h-vetinari approved these changes Jun 1, 2024
View reviewed changes
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

LGTM!

@ehfd
Copy link
Member Author

ehfd commented Jun 1, 2024

@h-vetinari You could merge if you want.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Jun 1, 2024

ehfd are you trying to also build packages for OSX and aarch?

Maybe we should add:

provider:
  linux_aarch64: default
build_platform:
  linux_aarch64: linux_64
  linux_ppc64le: linux_64
  osx_arm64: osx_64
conda_build:
  pkg_format: '2'
conda_forge_output_validation: true
test: native_and_emulated

in a file conda-forge.yml to build them on the first run optimistically.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Jun 1, 2024

similar to https://github.com/conda-forge/staged-recipes/pull/25992/files#diff-b06c1773820af0e7c38a9e9c53a478b75afd90b056775bd3c4c8cd950c6b8276 (but with ppc64le)

@ehfd
Copy link
Member Author

ehfd commented Jun 1, 2024

@hmaarrfk I agree because I had to add them in evdev after the feedstock got created. Just a second.

@ehfd
Copy link
Member Author

ehfd commented Jun 1, 2024 •
edited 

build_platform:
  linux_aarch64: linux_64
  linux_ppc64le: linux_64
  osx_arm64: osx_64
conda_build:
  error_overlinking: true
conda_forge_output_validation: true
github:
  branch_name: main
  tooling_branch_name: main

This seems to be the most recent version (default + build_platform from a few days ago). Added.

hmaarrfk
hmaarrfk reviewed Jun 1, 2024
View reviewed changes
conda-forge.yml Outdated
conda_forge_output_validation: true
github:
branch_name: main
tooling_branch_name: main
Copy link
Contributor

Choose a reason for hiding this comment

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

you should not edit this feedstock's file.

rather you need to create this file within the directory

recipes/xsel

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. OK.

Copy link
Member Author

@ehfd ehfd Jun 1, 2024 •
edited

Choose a reason for hiding this comment

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

That was what @xhochy said to me earlier. Didn't understand the first time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

lets see, its kinda a gamble if it works, but you are trying to get quite a few feedstocks in, so it might be helpful

hmaarrfk
hmaarrfk reviewed Jun 1, 2024
View reviewed changes
recipes/xsel/conda-forge.yml
conda_forge_output_validation: true
github:
branch_name: main
tooling_branch_name: main
Copy link
Contributor

@hmaarrfk hmaarrfk Jun 1, 2024 •
edited

Choose a reason for hiding this comment

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

Suggested change
tooling_branch_name: main
tooling_branch_name: main
conda_build:
pkg_format: '2'
test: native_and_emulated

Copy link
Member Author

@ehfd ehfd Jun 1, 2024 •
edited

Choose a reason for hiding this comment

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

I thought that pkg_format: '2' at least is the current default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyways, applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

Everything has been synced to all recipes as adequate. @hmaarrfk

Copy link
Member Author

Choose a reason for hiding this comment

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

It is with remorse that the "gamble" has no effect. I'll change the arches manually.

@hmaarrfk

ehfd and others added 2 commits June 1, 2024 23:05
@ehfd @hmaarrfk
Update recipes/xsel/conda-forge.yml
6e18ba7 
Co-authored-by: Mark Harfouche <mark.harfouche@gmail.com>
@ehfd
Minor edit
74bfa8f
@ehfd ehfd requested a review from hmaarrfk June 1, 2024 14:14
@hmaarrfk hmaarrfk merged commit 9256571 into conda-forge:main Jun 1, 2024
5 checks passed
@ehfd ehfd deleted the xsel branch June 1, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-vetinari h-vetinari h-vetinari approved these changes

@hmaarrfk hmaarrfk Awaiting requested review from hmaarrfk

Assignees
No one assigned
Labels
c-cpp review-requested
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ehfd @hmaarrfk @h-vetinari