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 in man page support for the different exposed functions with examples #139

Merged
merged 15 commits into from
Apr 9, 2018

Conversation

mrdeep1
Copy link
Collaborator

@mrdeep1 mrdeep1 commented Feb 12, 2018

./configure now supports --enable-manpages (on by default) where man pages are built and installed as appropriate.

'man coap' provides an overview

coap_attribute, coap_context, coap_handler, coap_recovery, coap_resource and coap_tls_library are in this update

Potentially, coap_logging, coap_session and coap_io have still to be written, and possibly others.

Copy link
Contributor

@tijuca tijuca left a comment

Choose a reason for hiding this comment

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

Please keep new entries in a alphabetical order to existing entries..

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 19, 2018

Do you mean new files added to be in an alphabetic order, or something else?

Copy link
Contributor

@tijuca tijuca left a comment

Choose a reason for hiding this comment

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

Just some remarks, otherwise I'm fine with this PR.

autogen.sh Outdated
@@ -27,6 +27,7 @@ depcomp
doc/Doxyfile doc/doxyfile.stamp doc/doxygen_sqlite3.db doc/Makefile doc/Makefile.in
examples/*.o examples/coap-client examples/coap-server examples/coap-rd
examples/coap-*.5 examples/coap-*.txt examples/Makefile.in
man/Makefile man/Makefile.in man/coap_*.[37] man/coap_*.txt man/coap_*.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep entries here in the alphabetical order.

configure.ac Outdated
man/coap_handler.txt
man/coap_recovery.txt
man/coap_resource.txt
man/coap_tls_library.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here please keep a alphabetical order of AC_CONFIG_FILES.

man/Makefile.am Outdated
@@ -0,0 +1,42 @@
# examples/Makefile.am
#
# Copyright (C) 2015 Carsten Schoenert <c.schoenert@t-online.de>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest you use your name for the copyright holder as you the author of the new file.

@tijuca
Copy link
Contributor

tijuca commented Feb 19, 2018

I hate this webgui ...
So, now with some comments on some changes.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 19, 2018

Understood - I will make changes and push shortly.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 19, 2018

Changes made and pushed. There was some other legacy out of order which I left alone.
Any other comments?

@tijuca
Copy link
Contributor

tijuca commented Feb 19, 2018

If ever possible avoid merge commits in pull requests, just rebase your changes on top of the devel branch instead and do a force push into your PR branch.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 19, 2018

OK - apologies for not getting it right. So in principal I should do something like - correct?
(in fork mrdeep1, which gave me the original develop)
git checkout develop
(pull in latest obgm/develop)
git pull upstream/develop
git checkout man-pages
git rebase develop
(fix things as appropriate)
git push -f origin man-pages

@tijuca
Copy link
Contributor

tijuca commented Feb 20, 2018

Yes. :-)
Just try is out. Git is smart enough to drop the merge commit(s) while doing the rebasing.

But I would also suggest here you do some more rebasing on your fixups and squash the fixups commits into the original commits.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 20, 2018

I have done some rebasing and squash fixups, so that there is now only the original log entry for autogen.sh, configure.ac and man/Makefile.am. How does it look now?

@tijuca
Copy link
Contributor

tijuca commented Feb 23, 2018

Then you have done something different than me. You should only see your patches on top of the devel branch an not any patches that are already exists on the branch devel.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 23, 2018

I am pretty sure I did the following (upstream is obgm/libcoap/develop):

git checkout develop
git pull upstream develop
git checkout man-pages
git rebase develop
.. then a series of rebasing, squashing 2 commits into 1
git push origin man-pages.

I think that as there were other commits on obgm/develop between my initial commit and updated commit - when I squashed the 2 together all the interim commits were rebuilt and hence became a part of my "git push origin manpages".

Other than my local repository says the only 'git diff' are what I expect from my changes, I so far have not found out how to do this through this GUI.

Is it worth me trashing this pull request and take some time re-doing it from scratch?

@tijuca
Copy link
Contributor

tijuca commented Feb 23, 2018

I've no idea what you have done as I've done exactly the same as you have written and I only had finally your patches in this branch in the end.
But any way, you don't need to do all from scratch you now can do again a rebasing and keep out all external commits that are not from you. That's one possibility, a second option would be to rename your branch (git branch -m man-pages-old), and create a new branch man-pages from the current head of the branch develop and do cherry-picking of the related commits afterwards.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 23, 2018

I think that what is now pushed from my end looks better. I did a "github rebase -i develop" and removed the unwanted commits.

@tijuca
Copy link
Contributor

tijuca commented Feb 24, 2018

Looks better now.
I haven't down own tests based (due lack of free time) on this PR but as this change set isn't interacting with the build of the binary stuff it should be o.k. if Olaf would merge this and we fix possible small side effects later. Thanks for adding the man pages!

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Feb 24, 2018

Thanks. My only comment is that it describes some call backs / functions that have pull requests outstanding. If I get some more spare cycles, I will try to do some of theother missing man pages.

@obgm
Copy link
Owner

obgm commented Feb 26, 2018

Thanks, @mrdeep1 and @tijuca, I will merge after the other outstanding PRs.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Mar 2, 2018

Updated for COAP_UNKNOWN_RESOURCE change in pull request #133

@mrdeep1 mrdeep1 force-pushed the man-pages branch 3 times, most recently from f96cc6f to 477bd84 Compare March 7, 2018 12:19
@obgm
Copy link
Owner

obgm commented Mar 7, 2018

Can we move the subfolder man into doc? Manuals are documentation, after all.


static void
init_resources(coap_context_t *ctx)
{
Copy link
Owner

Choose a reason for hiding this comment

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

I would like the examples to follow the coding guidelines of libcoap. Here, this means that the block-opening parenthesis is at the end of the previous line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As it happens, a2x does that anyway, but I will make the change. However, there have to be some blank lines to make things format, asthe a2x ' + ' fails tor work for me to start a new line.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Mar 7, 2018

I have seen man files being in the doc directory, man being in a separate directory from doc, but not man as a sub-directory of doc so far in other products.

I did not want to clutter the doc directory with the actual man pages, but can do this if you want.

@obgm
Copy link
Owner

obgm commented Mar 7, 2018

Ok, not sure. @tijuca do you have any preference?

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Mar 7, 2018

Updated with uri -> uri_path. Directory not changed.

@mrdeep1 mrdeep1 force-pushed the man-pages branch 2 times, most recently from 55d8311 to 3b89f76 Compare April 4, 2018 10:21
@obgm
Copy link
Owner

obgm commented Apr 5, 2018

Hm, it seems that the manual pages in man/ are built only if both, --enable-manpages and --enable-documentation are passed to configure (i.e., they are not built if any of these two is disabled). But although no manuals are build with configure --enable-manpages --disable-documentation, the configure script complains when a2x is not present. With configure --disable-manpages --enable-documentation, the manual pages in examples/ are built (which was the old behavior before --enable-manpages).

Maybe, we can make this a bit more consistent?
And we should probably move the manual pages from examples/ to man/?

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Apr 5, 2018

Hi Olaf,

I will try to dis-entangle the documentation building requirements. Would you have any objection to --enable-documentation to being both --enable-manpages + (new) --enable-doxygen ?
--disable-documentation would then override any --enable-manpages / --enable-doxygen definitions - OK?

Sure - happy to move examples man pages to man. I will leave the names with a - seperator, instead of _ separator as I have had to do the the functions man pages.

@obgm
Copy link
Owner

obgm commented Apr 5, 2018

Sounds good to me, thanks!

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Apr 5, 2018

I believe that I have got this working as expected, and fixed a couple of typos along the way in the man pages

*coap_attr_t *coap_find_attr(coap_resource_t *_resource_, const unsigned
char *_name_, size_t _nlen_);*

Link with -lcoap-1.
Copy link
Owner

Choose a reason for hiding this comment

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

We will run into trouble with burying the API number deep in the manual pages. How about something like

Link with  -lcoap-@LIBCOAP_API_VERSION@.

in the respective .in files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had held off changing the -l entry as there is the potential that it could be -llibcoap-2-openssl, etc. as a set of alternatives if the library renaming patch goes ahead.

Easy enough to change it to -lcoap-@LIBCOAP_API_VERSION@ for now.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, you are right. Usually pkg-config solves this but, of course, there are many platforms that do not have pkg-config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now -lcoap-@LIBCOAP_API_VERSION@in latest push

@mrdeep1 mrdeep1 force-pushed the man-pages branch 2 times, most recently from 2a47888 to e6941cd Compare April 7, 2018 10:19
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.

3 participants