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

build: add --disable-docs configure option #871

Merged
merged 1 commit into from Oct 25, 2016

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Oct 25, 2016

This may help address spack/spack#2109

@garlick garlick added the review label Oct 25, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 25, 2016

Current coverage is 72.10% (diff: 100%)

Merging #871 into master will decrease coverage by 0.02%

@@             master       #871   diff @@
==========================================
  Files           156        156          
  Lines         26957      26957          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19443      19436     -7   
- Misses         7514       7521     +7   
  Partials          0          0          

Powered by Codecov. Last update b7c84cf...8cf48c4

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2016

Coverage Status

Coverage decreased (-0.04%) to 75.715% when pulling 84c6ab4 on garlick:docs into 14b9ef1 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

This seems fine to me!
One Q -- what happens to flux help COMMAND when the manpages aren't available?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2016

One Q -- what happens to flux help COMMAND when the manpages aren't available?

Nothing special. Maybe we need a message indicating that flux wasn't built with documentation?

$ flux help flux_open
No manual entry for flux_open
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

Maybe we need a message indicating that flux wasn't built with documentation?

Yeah, building a real installation without some kind of builtin help kind of bothers me, but I could get over it. I think for now it is fine until someone complains. (?)

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 25, 2016

IMO, it sounds inevitable that someone will file a bug/issue saying flux help FOO doesn't work. I like the idea of just saying "no documentation built" to remove that possibility.

Edit: I should "inevitable" if this is how flux will be installed with Spack.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

Ok, @chu11, thanks for volunteering! 😸

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

There may be another issue too. I think flux-help is just execing man and relying on MANPATH set correctly to find the right man pages. In a case where there is system flux-core installed in default MANPATH, and a user runs a flux start from a non-standard flux of a different version, and that version is built without manpages, flux help will currently return the wrong man page, which could cause confusion.

Definitely flux help needs to ensure that it is going to find the right manpages before execing man, and "no documentation built" error is better than displaying man pages for the incorrect version.

I'm not sure @garlick or @chu11 if you had good ideas on how to handle this, but certainly a check before calling out to man is required.

There is also the idea to just install the asciidoc source somplace where it can be displayed with $PAGER when manpages were not built.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 25, 2016

Don't know code here that well, but perhaps INTREE_MAN_PATH is just not being loaded/pre-fixed properly? I can take a look more closely. Do you want to merge this one since it fixes the stated problem?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

Oops, I clicked "Update branch" accidentally, @garlick will have to rebase on current master anyway.

Don't know code here that well, but perhaps INTREE_MAN_PATH is just not being loaded/pre-fixed properly? I can take a look more closely. Do you want to merge this one since it fixes the stated problem?

It's true, this PR makes things no worse than they were, just allows build of flux-core at least on systems with broken a2x, so yeah we should probably merge this and I'll open an issue for flux help

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

Opened #874 for flux help enhancements being discussed here.

@garlick garlick force-pushed the garlick:docs branch from 4c5a0c9 to 8cf48c4 Oct 25, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2016

OK, rebased.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2016

Coverage Status

Coverage decreased (-0.03%) to 75.702% when pulling 8cf48c4 on garlick:docs into b7c84cf on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

Great. I'll merge once Travis passes

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2016

Coverage Status

Coverage decreased (-0.03%) to 75.709% when pulling 8cf48c4 on garlick:docs into b7c84cf on flux-framework:master.

@grondo grondo merged commit 342b127 into flux-framework:master Oct 25, 2016

4 checks passed

codecov/patch Coverage not affected when comparing b7c84cf...8cf48c4
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +27.87% compared to b7c84cf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 75.709%
Details

@grondo grondo removed the review label Oct 25, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2016

Not really related but on some systems (Ubuntu) setting manpath disables
system man pages....

On Oct 25, 2016 2:58 PM, "Mark Grondona" notifications@github.com wrote:

There may be another issue too. I think flux-help is just execing man and
relying on MANPATH set correctly to find the right man pages. In a case
where there is system flux-core installed in default MANPATH, and a user
runs a flux start from a non-standard flux of a different version, and
that version is built without manpages, flux help will currently return
the wrong man page, which could cause confusion.

Definitely flux help needs to ensure that it is going to find the right
manpages before execing man, and "no documentation built" error is better
than displaying man pages for the incorrect version.

I'm not sure @garlick https://github.com/garlick or @chu11
https://github.com/chu11 if you had good ideas on how to handle this,
but certainly a check before calling out to man is required.

There is also the idea to just install the asciidoc source somplace where
it can be displayed with $PAGER when manpages were not built.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.