Skip to content

Conversation

@bluca
Copy link
Contributor

@bluca bluca commented Feb 11, 2020

conf.py imports the version from the local module, so it needs to have the
directory in the python path, otherwise it fails with:

$ sphinx-apidoc -f -e -o docs/api msal
Creating file docs/api/msal.application.rst.
Creating file docs/api/msal.authority.rst.
Creating file docs/api/msal.exceptions.rst.
Creating file docs/api/msal.mex.rst.
Creating file docs/api/msal.token_cache.rst.
Creating file docs/api/msal.wstrust_request.rst.
Creating file docs/api/msal.wstrust_response.rst.
Creating file docs/api/msal.rst.
Creating file docs/api/msal.oauth2cli.assertion.rst.
Creating file docs/api/msal.oauth2cli.authcode.rst.
Creating file docs/api/msal.oauth2cli.oauth2.rst.
Creating file docs/api/msal.oauth2cli.oidc.rst.
Creating file docs/api/msal.oauth2cli.rst.
Creating file docs/api/modules.rst.
$ make -C docs man
make: Entering directory '/home/bluca/git/microsoft-authentication-library-for-python/docs'
Running Sphinx v1.8.4

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
File "/usr/lib/python3/dist-packages/sphinx/config.py", line 368, in eval_config_file
execfile_(filename, namespace)
File "/usr/lib/python3/dist-packages/sphinx/util/pycompat.py", line 150, in execfile_
exec_(code, _globals)
File "/home/bluca/git/microsoft-authentication-library-for-python/docs/conf.py", line 27, in
from msal import version as version
ModuleNotFoundError: No module named 'msal'

make: *** [Makefile:19: man] Error 2

conf.py imports the version from the local module, so it needs to have the
directory in the python path, otherwise it fails with:

$ sphinx-apidoc -f -e -o docs/api msal
Creating file docs/api/msal.application.rst.
Creating file docs/api/msal.authority.rst.
Creating file docs/api/msal.exceptions.rst.
Creating file docs/api/msal.mex.rst.
Creating file docs/api/msal.token_cache.rst.
Creating file docs/api/msal.wstrust_request.rst.
Creating file docs/api/msal.wstrust_response.rst.
Creating file docs/api/msal.rst.
Creating file docs/api/msal.oauth2cli.assertion.rst.
Creating file docs/api/msal.oauth2cli.authcode.rst.
Creating file docs/api/msal.oauth2cli.oauth2.rst.
Creating file docs/api/msal.oauth2cli.oidc.rst.
Creating file docs/api/msal.oauth2cli.rst.
Creating file docs/api/modules.rst.
$ make -C docs man
make: Entering directory '/home/bluca/git/microsoft-authentication-library-for-python/docs'
Running Sphinx v1.8.4

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/sphinx/config.py", line 368, in eval_config_file
    execfile_(filename, namespace)
  File "/usr/lib/python3/dist-packages/sphinx/util/pycompat.py", line 150, in execfile_
    exec_(code, _globals)
  File "/home/bluca/git/microsoft-authentication-library-for-python/docs/conf.py", line 27, in <module>
    from msal import __version__ as version
ModuleNotFoundError: No module named 'msal'

make: *** [Makefile:19: man] Error 2
@rayluo
Copy link
Contributor

rayluo commented Feb 11, 2020

Thanks for spending time on this, Luca!

You probably also noticed that we have a hand-written docs/index.rst, which is also hosted here. What would the outcome be, if you generate your man page just based on that?

We believe our hand-written API reference docs would be more organized than those blindly-generated api docs, because the latter one won't be smart enough to exclude internal helpers, hence resulting in a more-crowded, less-helpful doc.

@bluca
Copy link
Contributor Author

bluca commented Feb 11, 2020

Sorry I'm not that familiar with sphinx - what's the command to use that existing index?

@rayluo
Copy link
Contributor

rayluo commented Feb 11, 2020

Derived from sphinx's own doc, I ran this and, despite some warning, it seemed successful.

$ sphinx-build -b man docs output
Running Sphinx v2.4.1
making output directory... done
WARNING: html_static_path entry '_static' does not exist
building [mo]: targets for 0 po files that are out of date
building [man]: all manpages
updating environment: [new config] 15 added, 0 changed, 0 removed
reading sources... [100%] index
......
pickling environment... done
checking consistency... C:\Users\raluo\microsoft-authentication-library-for-python\docs\api\modules.rst: WARNING: document isn't included in any toctree
done
writing... msalpython.1 { } c:\users\raluo\microsoft-authentication-library-for-python\msal\application.py:docstring of msal.ClientApplication.__init__:: WARNING: more than one target found for cross-reference 'TokenCache': msal.token_cache.TokenCache, msal.TokenCache
done
build succeeded, 13 warnings.

The manual pages are in output.

PS: I am not that familiar with sphinx either. We just let our docs hosting service ReadTheDocs to do the heavy lifting. :)

@bluca
Copy link
Contributor Author

bluca commented Feb 11, 2020

That gives me the same error:

$ sphinx-build -b man docs output
Running Sphinx v1.8.4

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/sphinx/config.py", line 368, in eval_config_file
    execfile_(filename, namespace)
  File "/usr/lib/python3/dist-packages/sphinx/util/pycompat.py", line 150, in execfile_
    exec_(code, _globals)
  File "/home/bluca/git/microsoft-authentication-library-for-python/docs/conf.py", line 27, in <module>
    from msal import __version__ as version
ModuleNotFoundError: No module named 'msal'

With the attached patch the error is fixed. The diff between the manpage generated by that command and the ones I pasted in the original post is just one line and very minor:

$ diff -Naur docs/_build/man/msalpython.1 output/msalpython.1 
--- docs/_build/man/msalpython.1	2020-02-11 18:43:49.480649987 +0000
+++ output/msalpython.1	2020-02-11 18:43:26.344256998 +0000
@@ -263,7 +263,7 @@
 \fBvalidate_authority\fP (\fIbool\fP) \-\- (optional) Turns authority validation
 on or off. This parameter default to true.
 .IP \(bu 2
-\fBcache\fP (\fITokenCache\fP) \-\- Sets the token cache used by this ClientApplication instance.
+\fBcache\fP (\fI\%TokenCache\fP) \-\- Sets the token cache used by this ClientApplication instance.
 By default, an in\-memory cache will be created and used.
 .IP \(bu 2
 \fBverify\fP \-\- (optional)

@rayluo
Copy link
Contributor

rayluo commented Feb 11, 2020

Could that be a different behavior in sphinx itself? Would you mind upgrade your sphinx from 1.8.4 to its latest 2.4.1 and try again?

@bluca
Copy link
Contributor Author

bluca commented Feb 11, 2020

I cannot, Debian only has up to 1.8.5. Also, I don't really see how it could work - it's trying to import a module without having it in the path.
Are you sure that it's not just working for you because you have msal installed?

@rayluo
Copy link
Contributor

rayluo commented Feb 11, 2020

  • OK I tested it out, and you were right that, it worked on my machine because I had msal installed, and it would fail if I uninstall msal.
  • However, modifying sys.path on-the-fly is controversial that we should ideally avoid. In that sense, the root cause - which you also rightfully pointed it out - is the current conf.py is trying to import a module. And what's the canonical way of installing such a module? Usually, you just "pip install" it.
  • Do you have any special reason that you can not "pip install msal"? If you as an app developer would intend to use msal on your development machine, you would need to install it anyway. Unless, you are a man page builder, and you just build the man page and distribute it, without actually using msal by yourself. Is that the case here?

@bluca
Copy link
Contributor Author

bluca commented Feb 11, 2020

I am packaging this to be uploaded to Debian and Ubuntu: https://ftp-master.debian.org/new/microsoft-authentication-library-for-python_1.1.0-1.html
The package should ideally be self-bootstrappable. Installing itself to build itself is not the common practice.
In fact other Azure packages are following the same pattern from this PR, see for example: https://github.com/Azure/azure-cosmos-python/blob/master/doc/conf.py#L21

@bluca
Copy link
Contributor Author

bluca commented Feb 11, 2020

Also, installing a different version on the system means the docs are built using that version, rather than the one in the repository. I do not believe that is what you want.

@rayluo
Copy link
Contributor

rayluo commented Feb 11, 2020

To address your last comment, I believe it is solved on our docs building platform, because they do something equivalent to pip install -e .. That being said, it is fair enough to include this change. Merging.

Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks Luca!
Note: Turns out we need this, when/if the current machine did not already have an equivalent of pip install -e . to install msal beforehand.

@rayluo rayluo merged commit 94bc345 into AzureAD:dev Feb 11, 2020
@bluca bluca deleted the sphinx-path branch February 11, 2020 22:43
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.

2 participants