Skip to content

docs: do not set HTML install tree to 0755 perms #10258

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

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

jsquyres
Copy link
Member

@jsquyres jsquyres commented Apr 11, 2022

Follow-on to 8ffe33f: do not set the dirs in the installed HTML
files tree to have 0755 permissions because Automake doesn't do that
when it creates directories. Instead, use "find ..." to find all
directories and files in the generated _built/html tree and use the
Automake-provided installer macros to create the target directories
and install the target files with desireable permissions.

Additionally, add a comment explaining why the install-data-hook rule
exists, and why we are doing what we are doing in it.

Signed-off-by: Jeff Squyres [email protected]

To be added to #10255 when done.

@jsquyres jsquyres requested a review from bwbarrett April 11, 2022 20:48
@jsquyres jsquyres force-pushed the pr/dont-0755-install-dir-tree branch 2 times, most recently from afe234a to b46ae5c Compare April 11, 2022 23:18
@ibm-ompi
Copy link

The IBM CI (GNU/Scale) build failed! Please review the log, linked below.

Gist: https://gist.github.com/5f9117ae7324f71bd3162ad59e99d136

@ibm-ompi
Copy link

The IBM CI (XL) build failed! Please review the log, linked below.

Gist: https://gist.github.com/2b54fb084c71106c2656f34c39beb778

@ibm-ompi
Copy link

The IBM CI (PGI) build failed! Please review the log, linked below.

Gist: https://gist.github.com/acb9b4084be442a76e615b982cff50cf

@bwbarrett
Copy link
Member

The error in distcheck looks real, although I'm not entirely sure I understand why those files were readonly to begin with.

@jsquyres
Copy link
Member Author

jsquyres commented Apr 12, 2022

@bwbarrett Blah. This is what's happening:

  1. The generated man pages and HTML files are in the source tree (because it's doing a build of a tarball, so the tarball contains the pre-built man pages and HTML files).
  2. make distcheck does a chmod -R u-w $srcdir to make the source tree unwriteable
  3. Then distcheck does a configure+make+make install, which ends up doing cp -r $(srcdir)/_build/html $(DESTDIR)$(docdir) in the build tree docs dir

There's actually two problems here:

  1. The source tree docs/_build/html (and all of its subdirs) are r-xr-xr-x. When we cp -r SRC TARGET, even without the -p option, with a umask of 22, it ends up copying those permissions. For example:

    $ umask
    022
    $ mkdir src_tree
    $ touch src_tree/a-file
    $ chmod -R u-w src_tree
    $ ls -la src_tree 
    total 0
    dr-xr-xr-x. 2 jsquyres jsquyres 20 Apr 12 07:20 ./
    drwxr-xr-x. 4 jsquyres jsquyres 52 Apr 12 07:20 ../
    -r--r--r--. 1 jsquyres jsquyres  0 Apr 12 07:20 a-file
    $ cp -r src_tree dest_tree
    $ ls -la dest_tree 
    total 0
    dr-xr-xr-x. 2 jsquyres jsquyres 20 Apr 12 07:20 ./
    drwxr-xr-x. 5 jsquyres jsquyres 69 Apr 12 07:20 ../
    -r--r--r--. 1 jsquyres jsquyres  0 Apr 12 07:20 a-file
    

    Hence, when make uninstall invokes rm -rf THE_INSTALLED_DOCS_HTML_TREE, we get the permission denied errors.

    Since we're removing the entire tree anyway, probably the easiest fix here would be to add a chmod -R a+w THE_INSTALLED_DOCS_HTML_TREE before the rm -rf in the uninstall-hook rule.

    Sound right to you?

  2. The cp -r $(srcdir)/_build/html $(DESTDIR)$(docdir) action in the install-data-hook rule does the Wrong Thing when you're doing a VPATH build from a git clone (because the generated html tree is not in the source tree -- it's in the build tree). This doesn't show up in our testing because we only invoke distcheck, and in all of those cases, the generated HTML files are in the tarball/source tree; it only occurred to me to test this this morning when thinking through the above issues.

    I'll investigate what the Right Thing should be shortly.

@bwbarrett
Copy link
Member

Instead of chmoding around, why not replace the cp -r with a find -type f -exec $(install_sh) -c -m 644?

Follow-on to 8ffe33f: do not set the dirs in the installed HTML
files tree to have 0755 permissions because Automake doesn't do that
when it creates directories.  Instead, use "find ..." to find all
directories and files in the generated _built/html tree and use the
Automake-provided installer macros to create the target directories
and install the target files with desireable permissions.

Additionally, add a comment explaining why the install-data-hook rule
exists, and why we are doing what we are doing in it.

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres jsquyres force-pushed the pr/dont-0755-install-dir-tree branch from b46ae5c to 1867a72 Compare April 13, 2022 13:47
@jsquyres
Copy link
Member Author

@bwbarrett Done.

@bwbarrett
Copy link
Member

bot:aws:retest

@jsquyres jsquyres merged commit 796f46b into open-mpi:main Apr 13, 2022
@jsquyres jsquyres deleted the pr/dont-0755-install-dir-tree branch April 13, 2022 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants