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

Output appropriate error message on help failure and minor cleanups #887

Merged
merged 4 commits into from
Nov 1, 2016

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Oct 30, 2016

No description provided.

If flux-core is configured and built with --disable-docs, the "flux
help" command may still pick up manpages generated outside of the
current build (such as a version of flux installed on the system).

Install a simple ".nodocs" file when not generating documents.
When user attempts to user "flux help", this file can be detected
and an appropriate error message will be output to the user.

Fixes flux-framework#874
@garlick garlick added the review label Oct 30, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 75.754% when pulling e8f81cc on chu11:errordocsnotbuilt into 4801f62 on flux-framework:master.

For clarity, change HAVE_A2X automake conditional to ENABLE_DOCS.
Makes automake clearer across Makefile.am files and independent
of the tool used to generate manpages.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 75.75% when pulling 896d22f on chu11:errordocsnotbuilt into 4801f62 on flux-framework:master.

@chu11
Copy link
Member Author

chu11 commented Oct 30, 2016

Hmmm, lots of failures in make check, it appears a2x can't be found in travis

checking for a2x... no

probably have to add asciidoc to travis.yml. But before I do that, I don't see what could have happened between #871 and the current master.

@grondo
Copy link
Contributor

grondo commented Oct 30, 2016

We purposefully leave a2x out of Travis, it takes too long to install all
its dependencies. Why does make check fail without a2x now?

On Oct 30, 2016 3:41 PM, "Al Chu" notifications@github.com wrote:

Hmmm, lots of failures in make check, it appears a2x can't be found in
travis

checking for a2x... no

probably have to add asciidoc to travis.yml. But before I do that, I don't
see what could have happened between #871
#871 and the current
master.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#887 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAtSUp74vgduDxWjqCPRM-Y67zQ5LIcCks5q5R0TgaJpZM4KkZuj
.

@grondo
Copy link
Contributor

grondo commented Oct 30, 2016

Ah, the problem is (at the least) with the flux-help tests in t0001-basic.t. These tests use fake manpages to ensure flux help properly invokes man even when manpages aren't built, so the .nodocs file breaks these tests.

A possible workaround would be some kind of environment variable to disable the .nodocs test, or possibly point flux-help to a different config dir without the .nodocs file, etc... Then use that for any tests exercising flux-help

@chu11
Copy link
Member Author

chu11 commented Oct 30, 2016

Ahhh. I realized it was fake manpages in the tests, but I didn't quite get how things worked with a2x not being installed. I now see with the commit 0e13c83, the logic surrounding a2x being required/not required changed.

Yeah, I'll have to tweak the tests in some way.

@grondo
Copy link
Contributor

grondo commented Oct 30, 2016

I guess it would be safe if you removed .nodocs for the duration of those tests, if nothing else in make check is dependent on the .nodocs file (keep in mind someday parallel-tests might work, so multiple tests could be running at the same time)

Support workaround environment variable FLUX_IGNORE_NO_DOCS to
ignore .nodocs file and ensure flux-help unit tests can pass
regardless if manpages are generated or not.
@chu11
Copy link
Member Author

chu11 commented Oct 31, 2016

I elected to support a FLUX_IGNORE_NO_DOCS environment variable as a workaround. It's not the cleanest solution, but I think it ensures the unit tests will work regardless if docs are generated or not, and should be safe for any future parallelism in the testsuite in the future.

@chu11 chu11 changed the title Output appropriate error message on help failure and typo fix Output appropriate error message on help failure and minor cleanups Oct 31, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 75.771% when pulling 0030676 on chu11:errordocsnotbuilt into 4801f62 on flux-framework:master.

@codecov-io
Copy link

codecov-io commented Oct 31, 2016

Current coverage is 72.15% (diff: 66.66%)

Merging #887 into master will decrease coverage by 0.06%

@@             master       #887   diff @@
==========================================
  Files           156        156          
  Lines         26929      26935     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19448      19436    -12   
- Misses         7481       7499    +18   
  Partials          0          0          
Diff Coverage File Path
•••••• 66% src/cmd/builtin/help.c
•••••••••• 100% src/common/libflux/conf.c

Powered by Codecov. Last update 4801f62...0030676

@chu11
Copy link
Member Author

chu11 commented Oct 31, 2016

because of the workaround environment variable, code coverage does go down a tad as some of the new .nodocs code won't be touched.

@grondo
Copy link
Contributor

grondo commented Nov 1, 2016

Yeah, seems fine to me. Merging this one thanks!

@grondo grondo merged commit 53e1972 into flux-framework:master Nov 1, 2016
@grondo grondo removed the review label Nov 1, 2016
@chu11 chu11 deleted the errordocsnotbuilt branch June 5, 2021 16:58
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.

5 participants