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

Add "gepetto-viewer" recipe #12850

Merged
merged 2 commits into from
Oct 26, 2020
Merged

Add "gepetto-viewer" recipe #12850

merged 2 commits into from
Oct 26, 2020

Conversation

ymontmarin
Copy link
Contributor

@ymontmarin ymontmarin commented Oct 8, 2020

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.
  • 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

@conda-forge-linter
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/gepetto-viewer) and found it was in an excellent condition.

@ymontmarin ymontmarin changed the title Add "gepetto-viewer" with python biding recipe Add "gepetto-viewer" (with python bidings) recipe Oct 8, 2020
@ymontmarin
Copy link
Contributor Author

@conda-forge-admin, please restart ci

@conda-forge-linter
Copy link

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 (recipes/gepetto-viewer) and found some lint.

Here's what I've got...

For recipes/gepetto-viewer:

  • If python is a host requirement, it should be a run requirement.

@ymontmarin ymontmarin changed the title Add "gepetto-viewer" (with python bidings) recipe Add "gepetto-viewer" recipe Oct 11, 2020
@conda-forge-linter
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/gepetto-viewer) and found it was in an excellent condition.

@ymontmarin
Copy link
Contributor Author

ymontmarin commented Oct 11, 2020

@conda-forge/help-c-cpp

After the decision to focus on gepetto-viewer only in this recipe and after some work done on the X11 and OpenGL dependencies I finally managed to converge to a simple recipe that succeed to build Linux package.

However, after trying few workarounds, I cant manage to get it done for osx. I got the error:

/Users/runner/miniforge3/conda-bld/gepetto-viewer_1602451873289/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_plac/include/X11/Xlib.h:1686:1: error: unknown type name '_X_DEPRECATED'

during compilation. From what I found, the most similar error was in recipe discussion: conda-forge/ambertools-feedstock#18 which was also about a gui on MacOs. It seems related to the issue conda-forge/tk-feedstock#15 about interference between tk and X11 with tk overiding some X11 headers with incomplete ones.

I am not familiar with those kind of issue and already tried different fixes. Could you please help and advice on this issue ?

@ymontmarin
Copy link
Contributor Author

ping @jcarpent : update on recipe situation.

Do you accept to also be a maintainer ?

@jcarpent
Copy link
Contributor

Fine to be considered as a maintener

@ymontmarin
Copy link
Contributor Author

@conda-forge/help-c-cpp

Workaround found

After digging on a MacOS VM the behaviour of tk and X11 and how they install conflict headers, and all the conda-forge threads about this interaction, the cleanest warkaround i found is to alias the X11 of root include (which is corrupt by the tk installation which is required) to let the compiler use the host include folder X11 which is not corrupted. I restore the aliasing after compilation to respect folder structure for conda-build.

Next step

I think this PR is then ready to merge, @conda-forge/help-c-cpp

@ymontmarin
Copy link
Contributor Author

@conda-forge/help-c-cpp could it be possible to review the PR ? Recipe looks finish, checklist is done and CI is ok. Thanks !

@ymontmarin
Copy link
Contributor Author

@conda-forge/help-c-cpp , I squashed and rebase the PR branch to make it more readable for review.
I think the PR is ready for merge. Could it be possible to review the PR ? Thank you !

@jcarpent
Copy link
Contributor

@conda-forge/help-c-cpp We would need this package to be inside conda-forge before adding additional packages. Would you have time to review this PR and merge it?
Thanks in advance,
Best,

Justin

@jcarpent
Copy link
Contributor

@wolfv Sorry to directly ask you this, but we would need this PR to be merged, but we do not have any feedback from the maintainers. I guess they are all occupied, but if you have the rights, would you have time to handle this PR?
Thanks in advance for your help.

@jcarpent
Copy link
Contributor

@conda-forge-admin, please ping team

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@wolfv wolfv merged commit fa0bcc1 into conda-forge:master Oct 26, 2020
@wolfv
Copy link
Member

wolfv commented Oct 26, 2020

thanks for the contribution!

@ymontmarin
Copy link
Contributor Author

@wolfv Thank you for the merge !

@jcarpent
Copy link
Contributor

Thanks @wolfv for the quick merge.

@wolfv
Copy link
Member

wolfv commented Oct 26, 2020

btw @ymontmarin not sure if you deliberately chose gnu++11 vs c++11. Also the conda-forge default is C++17 on OS X and Linux. So that line might not be totally necessary.

@ymontmarin
Copy link
Contributor Author

@wolfv you are right ! I change it on the feedstock.

@ymontmarin ymontmarin deleted the add_gepetto branch October 30, 2020 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants