-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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 ogdf #22085
add ogdf #22085
Conversation
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 ( I do have some suggestions for making it better though... For recipes/ogdf:
|
Context and history about that package at #12498 (comment) |
@conda-forge/help-c-cpp, please review. The OSX error seems to be due to lower/uppercase mismatch in CMake (everything works fine on Linux). I tried a few things without success. If nobody has an idea, I'll simply disable osx before merging. |
extra: | ||
recipe-maintainers: | ||
- hadim | ||
- N-Coder |
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.
@N-Coder: are you interested in being a maintainer?
sha256: 308cc2749c6a63520f7979bac86a04066dfea2fb9d3a5e89831318db404bfbf5 | ||
|
||
patches: | ||
- cmake.patch |
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.
Maybe it would make the patch file easier to maintain (GitHub doesn't even want to display it online due to its size) if you only put actual changes to files' contents into the diff and then delete the respective folders in the script?
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 agree. Please only include the changes to the CMake files in this diff, and do the file deletions in the build script. Otherwise, the patch seems to be 99% file deletion.
Hey, here are some notes from my side...
|
sha256: 308cc2749c6a63520f7979bac86a04066dfea2fb9d3a5e89831318db404bfbf5 | ||
|
||
patches: | ||
- cmake.patch |
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 agree. Please only include the changes to the CMake files in this diff, and do the file deletions in the build script. Otherwise, the patch seems to be 99% file deletion.
commands: | ||
- test -f $PREFIX/lib/libOGDF.so # [linux] | ||
- test -f $PREFIX/lib/libOGDF.dylib # [osx] | ||
- if not exist %LIBRARY_LIB%\\OGDF.lib exit 1 # [win] |
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.
Please add an existence check for at least one file of each of the following categories: Cmake Config, Header,
- backward-cpp | ||
- pugixml | ||
- coincbc | ||
- libdwarf-dev |
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.
Please address static/underlinking warnings. We do not allow static linking.
- backward-cpp | ||
- pugixml | ||
- coincbc | ||
- libdwarf-dev |
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.
Looks like the errors on CMake for OSX are some combination of needing to provide CMake some better search paths, ogdf find_library implementations not working correctly for OSX, and/or the upstream packages not exporting the same CMake targets on all platforms.
-DWITH_EXTERNAL_PUGIXML=True \ | ||
${EXTRA_CMAKE_ARGS} | ||
|
||
make -j ${CPU_COUNT} |
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.
Add the --target option here and only build OGDF (the tests and example targets are unused).
I don't think I'll have time to maintain that repo. Someone should take the lead. |
If you need any help bringing a pre-built tmap package (i.e. wheel) to PyPI feel free to contact me. Doing that for the OGDF was pretty easy and anything that is available via pip is also available to users of conda (but not the other way around), so I don't really see a big need to invest too much work into abiding to the rules that are imposed on conda-forge. |
Conda forge has a lot of advantages compared to built binaries on PyPI. I just don't have the time to work on it, but ultimately, OGDF should be packaged on conda-forge. Thank you for your proposal on the TMAP wheel, but we already have an internal TMAP conda package that suits our needs for the moment. |
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).