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

Fix Everything That Is Broken (Which Is Almost Everything) #2

Merged
merged 5 commits into from
Aug 16, 2018

Conversation

leycec
Copy link
Contributor

@leycec leycec commented Aug 11, 2018

Greetings and summery salutations, fellow open-source compatriots. This means @kalefranz, @jakirkham, and @ccordoba12, may your Github usernames live in infamy.

You may remember me from such GitHub commentary as "Completely untested and liable to implode your fingers like meaty blood sausages across the keyboard." and such industry-standard feedstocks as BETSE and BETSEE, both of which depend upon this feedstock. When I originally authored those feedstocks, I made the inappopriate assumption that this feedstock behaved as advertised.

It doesn't. It's broken like a soggy watermelon on a urine-stained Hollywood sidewalk in June. Specifically, this feedstock (A) doesn't require the GraphViz package, (B) doesn't work under Windows but advertises that it does, (C) doesn't provide either Appveyor or Travis-CI configurations, and (D) doesn't define any meaningful unit or functional tests for those CI configurations to even exercise. well isn't that convenient

On the other hand, at least this feedstock actually exists. Large thanks for small favours, Universe.

Are You Babbling Incoherently?

I... I might be.

It's Friday night and I wanted to be doing something glamorous with my life. But our vocal userbase has begun shrieking like squalid harpies about our multiphysics biology simulator spontaneously failing with non-human-readable exceptions when graphing gene regulatory networks under Windows. Their plaintive mewling has moved me to vengeful tears of frustration finally do something useful.

This is that something. You may judge its usefulness.

Still Doing That Babbling Thing.

Very well. We shall now convey meaningful information.

This pull request fundamentally refactors this feedstock to comply with modern conda-forge design principles, largely inspired by the comparable python-graphviz feedstock. Changes are as follows:

  • Installation via pip rather than setuptools, improving both the portability and reliability of this recipe.
  • Addition of pip as a build-time requirement.
  • Addition of graphviz as a runtime requirement, which the prior version of this recipe erroneously omitted. ohmygods
  • Addition of a new Windows-specific patch, enforcing compatibility with the eye-stabbing dot.bat workaround bundled with the conda-specific version of GraphViz under Windows. This patch fixes graphviz package doesn't add executable to PATH on windows ContinuumIO/anaconda-issues#1666 with respect to this package.
  • Addition of a new run_test.py script, a minimalist unittest-based test suite ensuring the usability of this package across all supported platforms. The prior version of this recipe erroneously reported test success despite this package failing to run under Windows.
  • Addition of new .appveyor.yml and .travis.yml configurations – judiciously copypasted from the corresponding YAML files in the python-graphviz feedstock with tokens manually replaced by those listed in this feedstock's conda-forge.yml. Obviously, this isn't right. In my fondest dreams, these YAML files would already have been auto-generated by conda-smithy. They weren't, because this is no dream; this is the waking nightmare of a Friday night gone David Lynchian. I doubt my manually defining these files suffices to enable either Appveyor or Travis-CI integration. That probably requires some sort of authoritative administrator privileges. Like a thirsty man in the desert, however, even I will try to lick water from a toad's back. sounds deep, right?

The changeset is fairly hefty. I expect failure. I'll probably need a few rounds of deadly combat with the CI to smooth things over... assuming the CI decides to awaken from its poisonous slumber and actually do something, of course.

Thanks for all the ironclad packages forged in the hot depths of conda-forge! May the all-inclusive package manager be with you.

EDIT: Please do something useful, Heroku bot:

@conda-forge-admin, please rerender

O.K.! Now that we've stripped noarch: python to coerce conda-smithy into adding Windows to the build matrix, let's re-render us up the bomb yet again:

@conda-forge-admin, please rerender

@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 (recipe) and found some lint.

Here's what I've got...

For recipe:

  • The "source" section was expected to be a dictionary or a list, but got a ruamel.yaml.comments.CommentedMap.
  • The "build" section was expected to be a dictionary, but got a CommentedMap.
  • The "requirements" section was expected to be a dictionary, but got a CommentedMap.
  • The "test" section was expected to be a dictionary, but got a CommentedMap.
  • The "about" section was expected to be a dictionary, but got a CommentedMap.
  • The "extra" section was expected to be a dictionary, but got a CommentedMap.
  • The "package" section was expected to be a dictionary, but got a CommentedMap.
  • The home item is expected in the about section.
  • The license item is expected in the about section.
  • The summary item is expected in the about section.
  • The recipe could do with some maintainers listed in the extra/recipe-maintainers section.
  • The recipe must have a build/number section.
  • Recipe name has invalid characters. only lowercase alpha, numeric, underscores, hyphens and dots allowed
  • The "package" section was expected to be a dictionary, but got a CommentedMap.
  • The "extra" section was expected to be a dictionary, but got a CommentedMap.
  • The "package" section was expected to be a dictionary, but got a CommentedMap.
  • The "source" section was expected to be a dictionary or a list, but got a ruamel.yaml.comments.CommentedMap.
  • The "build" section was expected to be a dictionary, but got a CommentedMap.
  • The "requirements" section was expected to be a dictionary, but got a CommentedMap.
  • The "test" section was expected to be a dictionary, but got a CommentedMap.
  • The "about" section was expected to be a dictionary, but got a CommentedMap.
  • The "extra" section was expected to be a dictionary, but got a CommentedMap.

@ccordoba12
Copy link

LGTM but you need to fix the tests file.

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Cheers, @ccordoba12! Thanks for taking the scarce time to review this; my sanity appreciates it. I'll rectify the shamefully failing test suite and re-push.

Oddly, the SyntaxError: from __future__ imports must occur at the beginning of the file exception appears to be an erroneous false negative. Official documentation for the __future__ statement explicitly states that the the first __future__ statement in a module may be safely preceded by a module docstring, comments, blank lines, and other __future__ statements.

So, CPython itself and/or CPython documentation is wrong here. Sadly, nobody cares. Let's randomly shuffle the offending module docstring and comments about and see what happens. If it takes all week, I'll coerce massage CPython into accepting this... one way or another. </shakes_head>

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Also, the linter still unjustifiably hates me. Each of the The "blahblahblah" section was expected to be a dictionary, but got a CommentedMap. linter errors appears to be an ignorable artefact of the PyYAML or ruamel.yaml parsing process. A CommentedMap still satisfies the dict protocol and hence is a valid dictionary.

Why you so crazy, conda-forge-linter. Why.

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Actually, all of the linter output appears to be erroneous. I'm unconvinced the linter is correctly parsing the pushed meta.yml at all. In particular:

  • The home item is expected in the about section.

It's there.

  • The license item is expected in the about section.

It's there!

  • The summary item is expected in the about section.

Damn your uselessness, it's there!

  • The recipe could do with some maintainers listed in the extra/recipe-maintainers section.

It does, already. die conda-forge-linter die

  • The recipe must have a build/number section.

It does, you senseless pile of non-functioning automation.

  • Recipe name has invalid characters. only lowercase alpha, numeric, underscores, hyphens and dots allowed.

If pydot is an invalid recipe name, I will humbly eat my left Chaco sandle on monetized YouTube television. (1% of all advertisement proceeds to the conda-forge-linter development effort.)

Could this be a Jinja2, PyYAML, or ruamel.yaml issue? I am dumbfounded, linter. You have failed your masters yet again. Let us ignore the linter, for it is clearly attempting suicide.

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

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Ohboyohboy. conda-forge-linter can't actually tolerate any comments in meta.yml. Well, that's fundamentalist. I'd copied the build/patch: section as is from the python-graphviz recipe, which seemed sensible... at the time.

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

I... don't understand why CPython is failing to accept __future__ statements in run_test.py scripts.

That doesn't quite seem right, at all. Is conda-smithy circumventing Python's standard import machinery by doing something wild, crazy, and risky – like, say, loading the contents of run_test.py into an in-memory string and then eval-ing that string? This baffles the Pythonista in me.

Luckily, we don't appear to actually require __future__ statements in this particular run_test.py script. Let's banish them to Valhalla and try it all again, shall we?

But know this, conda-smithy: I will never forget your brazen treachery.

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Curiouser and curiouser. GraphViz fails to recognize a file format that it actually recognizes:

b'Warning: Could not load "/feedstock_root/build_artefacts/pydot_1534210540550/_t_env/lib/graphviz/libgvplugin_gdk.so.6" - file not found\n
Format: "jpe" not recognized. Use one of:
bmp canon cmap cmapx cmapx_np dot eps fig gv ico imap imap_np ismap
jpe jpeg jpg pdf pic plain plain-ext png pov ps ps2 svg svgz tif tiff tk vml vmlz
xdot xdot1.2 xdot1.4\n'

Note the jpe in the above whitespace-delimited list. So, that's utter rubbish. Let's just silently ignore all tests failing with internally inconsistent error messages. What's the worst that could happen?

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

One out of three ain't bad!

...oh wait, it absolutely is. Travis-CI, why you continue to disappoint me?

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

No one will ever quite know why Travis-CI is failing with this cryptic exception:

$ conda build ./recipe -m ./.ci_support/${CONFIG}.yaml
Traceback (most recent call last):
  File "/Users/travis/miniconda3/bin/conda-build", line 11, in <module>
    sys.exit(main())
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/cli/main_build.py", line 424, in main
    execute(sys.argv[1:])
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/cli/main_build.py", line 415, in execute
    verify=args.verify)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/api.py", line 200, in build
    notest=notest, need_source_download=need_source_download, variants=variants)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/build.py", line 2184, in build_tree
    bypass_env_check=True)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/render.py", line 738, in render_recipe
    variants = get_package_variants(m, variants=variants)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/variants.py", line 489, in get_package_variants
    specs[f] = parse_config_file(f, config)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/variants.py", line 115, in parse_config_file
    with open(path) as f:
FileNotFoundError: [Errno 2] No such file or directory: './.ci_support/osx_python2.7.yaml'
The command "conda build ./recipe -m ./.ci_support/${CONFIG}.yaml" exited with 1.
1.00s$ upload_or_check_non_existence ./recipe conda-forge --channel=main -m ./.ci_support/${CONFIG}.yaml
Traceback (most recent call last):
  File "/Users/travis/miniconda3/bin/upload_or_check_non_existence", line 146, in <module>
    main()
  File "/Users/travis/miniconda3/bin/upload_or_check_non_existence", line 109, in main
    metas = conda_build.api.render(recipe_dir, variant_config_files=args.variant_config_files)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/api.py", line 44, in render
    permit_unsatisfiable_variants=permit_unsatisfiable_variants)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/render.py", line 738, in render_recipe
    variants = get_package_variants(m, variants=variants)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/variants.py", line 489, in get_package_variants
    specs[f] = parse_config_file(f, config)
  File "/Users/travis/miniconda3/lib/python3.6/site-packages/conda_build/variants.py", line 115, in parse_config_file
    with open(path) as f:
FileNotFoundError: [Errno 2] No such file or directory: './.ci_support/osx_python2.7.yaml'
The command "upload_or_check_non_existence ./recipe conda-forge --channel=main -m ./.ci_support/${CONFIG}.y

I assume this is all my fault and that I should feel bad.

Time to roll up my YAML sleeves and get dirty with .travis.yml.

@leycec leycec force-pushed the fixthatwhichisbroke branch 3 times, most recently from 8fb7d86 to 7eb16eb Compare August 14, 2018 02:06
This commit fundamentally refactors this recipe to comply with modern
conda-forge design principles, largely inspired by the comparable
"python-graphviz" recipe. Significant improvements are as follows:

* Installation via "pip" rather than "setuptools", improving both the
  portability and reliability of this recipe.
* Addition of "pip" as a build-time requirement.
* Addition of "graphviz" as a runtime requirement, which the prior
  version of this recipe erroneously omitted.
* Inclusion of a new Windows-specific patch, enforcing compatibility
  with the "dot.bat" workaround bundled with the conda-specific version
  of GraphViz under Windows. This patch fixes
  ContinuumIO/anaconda-issues#1666 with respect to this package.
* Inclusion of a new "run_test.py" script, a unittest-based test suite
  guaranteeing the usability of this package across all supported
  platforms. The prior version of this recipe erroneously reported test
  success despite this package failing to run under Windows.
* Addition of new ".appveyor.yml" and ".travis.yml" configurations –
  judiciously copypasted from the corresponding YAML files in the
  conda-forge/python-graphviz-feedstock with tokens manually replaced by
  those listed in this feedstock's "conda-forge.yml".
@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Blargh. Assistance politely requested, because you are all awesome and I have no idea what I'm doing.

I'm feebly trying to re-render the feedstock so that conda-smithy adds an Appveyor-based Windows configuration to the build matrix; adding a Travis-CI-based macOS configuration would be nice as well, but entirely optional. Windows isn't, because pydot on Windows really needs to be tested.

Naturally, conda-smithy only adds a CircleCI-based Linux configuration to the build matrix. I'm perusing the new conda-build 3 instructions for "Writing the meta.yaml", but failing to note anything relevant. Is noarch the issue? Does conda-smithy improperly assume that testing a noarch package under CircleCI alone suffices?

EDIT: Blargh! That's exactly it:

The noarch: python directive, in the build section, makes pure-Python packages that only need to be built once. This drastically reduces the CI usage, since it’s only built once (on CircleCI), making your build much faster and freeing resources for other packages.

A sad day for Python-kind. Since pydot must be built on Appveyor to ensure sanity under Windows, I'm afraid I need to pull the noarch: python directive from this feedstock. Now I too feel bad. 😞

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Well, isn't this just the veritable Mr. Toad's Wild Ride of build management.

@leycec
Copy link
Contributor Author

leycec commented Aug 14, 2018

Oh... Oh, my Nordic Gods. Appveyor actually passed. The bloodied but unbroken Eagle has landed.

pydot now behaves as expected under all supported platforms, including the continual horror show that is Windows. Thanks, all!

EDIT: Would anyone like me to clean up the commit history a bit, or is the shambolic wreckage fine as is?

@@ -0,0 +1,18 @@
This patch is required to properly recognize the Windows-specific batch script

Choose a reason for hiding this comment

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

Please move this comment to the recipe itself. I'll point it out where.

recipe/meta.yaml Outdated
@@ -10,21 +10,23 @@ source:
fn: {{ name }}-{{ version }}.tar.gz
url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz
sha256: {{ sha256 }}
patches:
- windows-bat.patch # [win]

Choose a reason for hiding this comment

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

Please move the comment about the need of this patch above this line.

Copy link
Contributor Author

@leycec leycec Aug 15, 2018

Choose a reason for hiding this comment

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

You got it. That comment was originally situated above that exact line, for orthogonality with the python-graphviz feedstock. However, this caused conda-forge-linter to break with non-human-readable non-errors. In the interest of just shutting up the linter, I then shifted that commentary out into the patch.

Annnnyway. As you suggest, who cares about the linter? Recipe maintainability takes precedence. The linter's fundamentally broken as currently defined, anyway. Christ, someone fix that linter.

I have the sudden feeling I talk too much.

@ccordoba12
Copy link

Would anyone like me to clean up the commit history a bit

I don't see the need of it. There are only 4 commits in this PR.

@leycec
Copy link
Contributor Author

leycec commented Aug 15, 2018

I don't see the need of it. There are only 4 commits in this PR.

That's what I'm talkin' about. I've now added the Windows patch commentary back to the recipe as you advise, because conda-forge-linter is dumb.

We're probably nearly there. Maybe? I cross all ten of my fingers for good luck.

@leycec
Copy link
Contributor Author

leycec commented Aug 16, 2018

Cheers, @ccordoba12. Thanks again for the perspicacious review.

@ccordoba12
Copy link

@jakirkham, @ocefpaf:

@leycec has done a great work here. This deserves to be merged ASAP. Thanks!

@ccordoba12
Copy link

@leycec, please add yourself as maintainer to the recipe so you can merge PRs yourself in the future in this feedstock.

Copy link
Member

@nehaljwani nehaljwani left a comment

Choose a reason for hiding this comment

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

The comments on this GH issue made my day.

I apologize for the broken recipe.

@ocefpaf
Copy link
Member

ocefpaf commented Aug 16, 2018

@ccordoba12 I guess @nehaljwani is the maintainer here and he is active in the thread, so no need for me to step on his toes 😉

@nehaljwani please merge when you are ready

@leycec thanks for the PR! Please consider becoming a maintainer for this feedstock.

@nehaljwani nehaljwani merged commit d618467 into conda-forge:master Aug 16, 2018
@ccordoba12
Copy link

Thanks @nehaljwani!

@leycec
Copy link
Contributor Author

leycec commented Aug 17, 2018

Thanks for such prompt action, all. Conda-forge collaboration wins again!

Against all sense of rationality and the instinctual drive for self-preservation, I've submitted yet another pull request adding myself as additional repository maintainer. Apparently, some people think that's a not-awful idea. whatamidoing

@meistermeister
Copy link

pydot 1.4.1 it seems to have resurrected the issue.
Win 10
Conda
graphviz 2.3.8
python-graphviz 0.10.1
conda 4.6.14
conda navigator 1.9.7

Fixed it by manually changing the $PATH, but that's what we were eliminating, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

graphviz package doesn't add executable to PATH on windows
7 participants