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

docs: Get rid of badly named readme #15176

Merged
merged 1 commit into from Jan 31, 2019

Conversation

Projects
None yet
4 participants
@merland
Copy link
Contributor

commented Jan 16, 2019

With its current name, the file doc/README_osx.md looks like an entry point README for OSX users, but it only contains specific instructions on how to build a DMG.

This PR deletes the file and moves the contents of the file into doc/build-osx.md.

@merland merland changed the title Rename README_osx.md doc: Rename README_osx.md Jan 16, 2019

@fanquake fanquake added the Docs label Jan 16, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

As these are build instructions, maybe it would make sense to integrate it into the existing build_osx.md?
I don't think the README_ prefix makes much sense in the documentation directory.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@laanwj I was thinking the same, but since doc/release-process.md links to the DMG instructions, it could make sense to keep them in a separate file. Alternatively link to an anchor inside build_osx.md.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

NACK renaming to _dmg.

Even README_windows.txt could be removed and integrated into doc/README.md#Windows. These are the only "special" variants of the README..

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Well the point of README_windows.txt is as an introductionary text for windows users. That one makes sense. It's also copied into the windows archives.

@merland's point is that this text is not an introductionary text for OSX users so is named wrongly.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Thanks, I wasn't aware of that. It has kind of duplicate content but maybe https://github.com/bitcoin/bitcoin/blob/master/doc/README.md#windows should point to README_windows.md?

I suggest to somehow inline README_osx.md in build_osx.md or to rename to build_osx_dmg.md.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Regarding README_windows.txt I also argue for its removal, due to out-of-context info and duplication. I actually tried to remove it before (#14755) but that failed the Gitian build. Anyway, I think it would be better to handle that in a separate PR.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Regarding README_windows.txt I also argue for its removal, due to out-of-context info and duplication

It's actually used! so no, I'd argue not to remove it! Yes, there is some duplication, but also some things that are different for windows. If you'd remove it you'd have to replace it with something else similar anyhow (which is used as an introductionary text for windows users). Feel free to rewrite it though.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@laanwj Yes, I know the file is used but I have a feeling we can find a better strategy. One that satisfies both those who read the archive and those who read from GitHub, with no build script repackaging.

My best idea for a compromise would be to keep the file but have it contain only a reference to the windows anchor in the main doc/README.md. Thoughts on that?

Related: #15161

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

No, that won't work because the doc/README.md is not copied in the same way!
doc/README.md is the Table of Contents of the doc file. I guess that would work if the entire folder is copied, but it'd also be confusing to new users because that folder contains developer documentation as well.

So the file should stand on its own. I think it's fine, let's just not change this okay.

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

Ok, there is obviously something I'm missing here. Where can I learn more about how the docs are copied/packaged?

@promag

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

See

bitcoin/Makefile.am

Lines 54 to 57 in d44b01f

WINDOWS_PACKAGING = $(top_srcdir)/share/pixmaps/bitcoin.ico \
$(top_srcdir)/share/pixmaps/nsis-header.bmp \
$(top_srcdir)/share/pixmaps/nsis-wizard.bmp \
$(top_srcdir)/doc/README_windows.txt

# Installer sections
Section -Main SEC0000
SetOutPath $INSTDIR
SetOverwrite on
File @abs_top_srcdir@/release/@BITCOIN_GUI_NAME@@EXEEXT@
File /oname=COPYING.txt @abs_top_srcdir@/COPYING
File /oname=readme.txt @abs_top_srcdir@/doc/README_windows.txt

@merland merland force-pushed the merland:rename-osx-readme branch Jan 17, 2019

@merland merland changed the title doc: Rename README_osx.md docs: Get rid of badly named readme Jan 17, 2019

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

Updated commit, title and description. Please re-review!

(Note that the lengthy discussion about README_windows.txt above can be ignored completely in the context of the proposed changes.)

@merland

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@laanwj @promag Re-review after update?

doc/release-process.md Outdated
@@ -92,7 +92,7 @@ Ensure gitian-builder is up-to-date:
echo 'f9a8cdb38b9c309326764ebc937cba1523a3a751a7ab05df3ecc99d18ae466c9 inputs/osslsigncode-1.7.1.tar.gz' | sha256sum -c
popd

Create the macOS SDK tarball, see the [macOS readme](README_osx.md) for details, and copy it into the inputs directory.
Create the macOS SDK tarball, see the [OSX build instructions](build-osx.md#deterministic-macos-dmg-notes) for details, and copy it into the inputs directory.

This comment has been minimized.

Copy link
@promag

promag Jan 20, 2019

Member

Actually it is macOS, it was decided to not rename the file IIRC.

This comment has been minimized.

Copy link
@merland

merland Jan 21, 2019

Author Contributor

@promag Thanks for reviewing. Could you please elaborate? This PR does not propose a rename (after the update).

This comment has been minimized.

Copy link
@promag

promag Jan 21, 2019

Member

I mean you should keep macOS.

This comment has been minimized.

Copy link
@merland

merland Jan 21, 2019

Author Contributor

Ah, I see it now. Will update.

This comment has been minimized.

Copy link
@merland

merland Jan 21, 2019

Author Contributor

updated

@merland merland force-pushed the merland:rename-osx-readme branch to f24ed6d Jan 21, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

utACK f24ed6d

@laanwj laanwj merged commit f24ed6d into bitcoin:master Jan 31, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 31, 2019

Merge #15176: docs: Get rid of badly named readme
f24ed6d Delete README_osx.md and move its contents into build-osx.md (Martin Erlandsson)

Pull request description:

  With its current name, the file `doc/README_osx.md` looks like an entry point README for OSX users, but it only contains specific instructions on how to build a DMG.

  This PR deletes the file and moves the contents of the file into `doc/build-osx.md`.

Tree-SHA512: 2636b9da967f2a4c0d68cb9a157fb3db137bdb8fbff5d9d004f28b5d816e9c27fddc5c403e6b0c363d1dc9ddc7cac8b295efa01fc691126c0e36e21bb9b3cbd3

@merland merland deleted the merland:rename-osx-readme branch Apr 15, 2019

@merland merland restored the merland:rename-osx-readme branch Apr 15, 2019

@merland merland deleted the merland:rename-osx-readme branch Apr 15, 2019

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.