-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add xorg-xft #2592
Add xorg-xft #2592
Conversation
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/xorg-xft:
|
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 ( |
Sorry for the tardiness here — sure, I'd be happy to be a maintainer. Although I haven't read over the recipe just yet ... |
@pkgw This is really just more of the same. I added a couple of packages building sources from xorg (walking down the dependency chain for a tool I'd like to add to bioconda). xorg-printproto just contains the header files for the print protocol. xorg-xbitmaps the X shared bitmap header files in include/X11/bitmaps. xorg-libxft is the X font library, xorg-libxp the print library. Since all of those are from x.org, I figured I'd stick to the scheme you used for the other x.org packages and wanted to add you as maintainer in case you need/want to do wholesale changes to the collection of packages later on. Do you know what I need to do to get these merged? |
Required fontconfig is not available for windows
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.
There are a few things in the recipe that should be fixed.
recipes/xorg-xft/build.sh
Outdated
make install | ||
make check | ||
|
||
rm -rf $uprefix/share/man $uprefix/share/doc/libXext |
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 line should be updated to remove any files associated with xft that do not need to be distributed. Usual practice is to remove documentation files.
recipes/xorg-xft/build.sh
Outdated
|
||
# Non-Windows: prefer dynamic libraries to static, and dump libtool helper files | ||
if [ -z "VS_MAJOR" ] ; then | ||
for lib_ident in Xext; do |
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.
Does this package install any shared libraries? If so, this line should be changed to only affect the shared libraries installed by this package. I.e., if this package installs libXfoo.dylib
and libXbar.dylib
on OSX, the loop should be for lib_ident in Xfoo Xbar ; do
.
recipes/xorg-xft/meta.yaml
Outdated
sha256: {{ sha256 }} | ||
|
||
build: | ||
number: 3 |
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 should be zero for a new package.
recipes/xorg-xft/meta.yaml
Outdated
- xorg-util-macros | ||
- xorg-libpthread-stubs | ||
- xorg-libxrender 0.9.* | ||
- freetype |
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 freetype
and fontconfig
lines should specify the version pinnings as listed in this obscure data file, in both the "build" and "runtime" dependency sections.
recipes/xorg-xft/meta.yaml
Outdated
|
||
extra: | ||
recipe-maintainers: | ||
# - pkgw |
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 is OK to uncomment.
@epruesse, I have pointed out some changes in the recipe that should be made. Once you're happy with things, you should make a comment directed at @conda-forge/core to ping the core team and ask them to take another look and potentially merge the PR if it looks good. |
- remove docs (only has man files) - remove static libs on windows - reset build number to 0 - pin fontconfig and freetype - add @pkgw as maintainer
@conda-forge/core Could you have a look at the recipe and merge if all looks good? |
@pkgw I don't think I can @mention private teams I'm not a member of. At least, github doesn't turn the @conda-forge/core mention into a link for me. |
Interesting, I didn't know that was a thing. But you certainly typed the team name right ... @jakirkham @ocefpaf @jjhelmus etc, can anyone take a look at this? |
@@ -0,0 +1,55 @@ | |||
{% set xorg_name = "libXft" %} |
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 this is going to be libXft
, then the directory needs to be xorg-libxft
. This means the package will also be named xorg-libxft
. Is the lib
something you want in there or not? How do others name this package? Part of the reason I ask is this hasn't been converted because of this naming issue. Thoughts @epruesse @pkgw?
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.
D'oh. I think the ideal practice is to track the names of the X.org packages as closely as possible, in which case yes, this directory should have been named xorg-libxft
.
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.
Darn. Didn't see that. Should I create a new PR or update this one?
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 just created #2718 which changes the recipe folder name to xorg-libxft
@pkgw Would you mind being recipe-maintainer? I used your xorg-* packages as template for xft.