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 doxygen #2135

Merged
merged 32 commits into from
Jan 5, 2017
Merged

Add doxygen #2135

merged 32 commits into from
Jan 5, 2017

Conversation

shadowwalkersb
Copy link
Contributor

@shadowwalkersb shadowwalkersb commented Dec 28, 2016

Closes #1733 .

  • Check license info

@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/doxygen) and found it was in an excellent condition.

@shadowwalkersb
Copy link
Contributor Author

Need a review and help here.
Windows is set to skip, so isn't it supposed to be skipped for CI?
CMakeLists has flex and bison set as required. When added as build req in the recipe Mac builds fail with conda-forge versions of flex and bison. When removed Mac builds find ones existing in the system and builds successfully, but Linux builds fail. Workaround I found is to remove flex and bison from build requirements and add them into yum_requirements.txt.

@jakirkham
Copy link
Member

That's unfortunate. Sounds like we may have a bug in cmake then. 😕 Could you please raise an issue on that feedstock with some info about how to reproduce the issue?

@shadowwalkersb
Copy link
Contributor Author

shadowwalkersb commented Dec 30, 2016

I think cmake is good. When flex and bison are listed in build reqs, they are installed and cmake finds them. If not listed as build req, it cmake finds the system ones on Mac and fails to find them on Linux. But, when they are installed via yum_requirements.txt, then cmake finds them. IMO, problem is with conda-forge's flex and/or bison. Not sure when I can dig up the logs and report on flex and bison feedstocks.

@shadowwalkersb
Copy link
Contributor Author

Or, I could demonstrate the problem here in this PR. Would you rather keep this PR clean and merge it as is, maybe? The failure on Appveyor is another story. conda-build complains about the license. So, I just skipped Windows, but it is still built. I am not sure if that is, yet another problem. So, we got a stack of problems in our hands here. 😜

@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/doxygen) and found some lint.

Here's what I've got...

For recipes/doxygen:

  • The license item is expected in the about section.

-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DCMAKE_BUILD_TYPE=Release \
..
make -j
Copy link
Member

Choose a reason for hiding this comment

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

Please add $CPU_COUNT if you wish to build in parallel.

Copy link
Contributor Author

@shadowwalkersb shadowwalkersb Dec 31, 2016

Choose a reason for hiding this comment

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

Not building in parallel. Done.

- vc14 # [win and py35]

requirements:
build:
Copy link
Member

Choose a reason for hiding this comment

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

Needs python as a build requirement on Windows for features to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And if want to skip Windows, should I remove features? Or can they stay for future use?

Copy link
Member

Choose a reason for hiding this comment

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

They can stay. Just suggesting we get all the pieces in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.


test:
commands:
- test -f "${PREFIX}/bin/doxygen" # [not win]
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we just run doxygen --help and drop the selector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

- test -f "${PREFIX}/bin/doxygen" # [not win]

about:
home: http://www.stack.nl/~dimitri/doxygen/index.html
Copy link
Member

Choose a reason for hiding this comment

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

Please add license. It looks like it is GPL2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the license_family?

Copy link
Member

Choose a reason for hiding this comment

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

That's optional. Sadly it is also GPL2 so a bit redundant. Would skip it if I were you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jakirkham
Copy link
Member

The license complaint is valid. So that needs to be addressed.

I'd really like to understand the issues with flex and bison before we proceed. Right now I don't follow. Could you please demonstrate the issue?

@shadowwalkersb
Copy link
Contributor Author

The license complaint is valid. So that needs to be addressed.

Only Windows complains, the rest passes though. I'll add GPL2 for license field as you suggested.

@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/doxygen) and found it was in an excellent condition.

@shadowwalkersb
Copy link
Contributor Author

Mac and Linux fail with this message:
[ 24%] [BISON][constexp] Building parser with bison 3.0.4 /staged-recipes/build_artefacts/doxygen_1483153761659/_b_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_plac/bin/bison: m4 subprocess failed

@isuruf
Copy link
Member

isuruf commented Jan 4, 2017

How about updating test 12 using --updateref --id 12 and then running all the tests?

@shadowwalkersb
Copy link
Contributor Author

I can't follow the code. What is it supposed to do?

@shadowwalkersb
Copy link
Contributor Author

Here's a brainteaser for you. Why does pinning conda-build to 2.0.12 fix builds? I'm pretty sure, I saw successful builds with 2.1 in this PR, but after some point things started getting weird. I'm giving up for now. 😩 😫

@isuruf
Copy link
Member

isuruf commented Jan 4, 2017

It will first update the test 12 with the output from doxygen.
When make tests is run again, it will obviously match because we have updated the expected output.

python runtests.py --updateref --id 12 --inputdir=testing
make tests

@@ -19,7 +19,7 @@ conda install --yes --quiet conda-forge-build-setup
source run_conda_forge_build_setup

# install conda-build 2.x to build with a long prefix
conda install --yes --quiet conda-build=2
conda install --yes --quiet conda-build=2.0.12
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this repo is using conda-build=2, even though that line is there. Previously it was downgraded to 1.21.

The following packages will be DOWNGRADED due to dependency conflicts:

    conda-build: 2.1.0-py35_0 defaults --> 1.21.14-py35_0 defaults

When pinned to 2.0.12 instead of 2, it actually works.
cc @ocefpaf

Copy link
Member

Choose a reason for hiding this comment

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

That was a mistake, it was downgraded and then upgraded.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's happening when running source run_conda_forge_build_setup. This line is after that so it shouldn't matter what conda-forge-build-setup does.

@isuruf
Copy link
Member

isuruf commented Jan 4, 2017

I couldn't find a build log with conda-build 2.1 that worked.

@shadowwalkersb
Copy link
Contributor Author

You're right, I can't find any either. So, I guess, it is safe to say that conda-build 2.1 breaks things here, but 2.0.12 is OK?

@isuruf
Copy link
Member

isuruf commented Jan 4, 2017

Yes, it's strange though that 2.1 breaks stuff. I've managed to compile with 2.1 by adding,
export LDFLAGS="${LDFLAGS} -L${PREFIX}/lib -liconv" at the top of build.sh (This makes me more puzzled.)

@jakirkham
Copy link
Member

It is very possible there is a bug or at least an unexpected change of behavior. Could you guys please raise an issue on the conda-build issue tracker? Feel free to xref back to this PR.

@isuruf
Copy link
Member

isuruf commented Jan 4, 2017

This seems to be a bug in doxygen and the bug was hidden in conda-build < 2.1 and came to surface in conda-build 2.1. conda-build 2.1 probably has a change related path (a good change that found this bug.)
Previously it was finding iconv_open function from glibc and iconv.h from glibc as well. Now due to some path related change, iconv.h is found from libiconv conda package, but iconv_open is still found from glibc. I have a PR that fixes this mismatch

@isuruf
Copy link
Member

isuruf commented Jan 4, 2017

shadowwalkersb/staged-recipes#2

@shadowwalkersb
Copy link
Contributor Author

@isuruf , would you be interested in being a maintainer? Maybe, even the only maintainer?

@isuruf
Copy link
Member

isuruf commented Jan 5, 2017

Sure, I can help with maintaining

@shadowwalkersb
Copy link
Contributor Author

To be clear, if you don't mind, I'll remove myself.

@shadowwalkersb
Copy link
Contributor Author

@jakirkham , @isuruf played it nicely, and kindly fixed CMakelLists, but I just removed the problematic part from CMakeLists. Even if it works, please, let me know if you want me to back out my changes. Considering @isuruf is going to be the only maintainer, it may be better to go with his solution, but I still wanted to put it out there. 😉

@shadowwalkersb
Copy link
Contributor Author

I, just, need a final confirmation that the license is GPL2, please.

@isuruf
Copy link
Member

isuruf commented Jan 5, 2017

Yes, it's GPL2. Btw, there was a new release doxygen-1.8.13 after this PR was open.

@isuruf
Copy link
Member

isuruf commented Jan 5, 2017

Ready to merge now.

@scopatz scopatz merged commit 98b4dc7 into conda-forge:master Jan 5, 2017
@scopatz
Copy link
Member

scopatz commented Jan 5, 2017

Thanks all!

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.

5 participants