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

Simplify readme #5217

Merged
merged 3 commits into from Aug 20, 2019

Conversation

@pazos
Copy link
Contributor

commented Aug 16, 2019

Work in progress, related to #5213

My main problem is with english, please help :octocat:

@Frenzie
Copy link
Member

left a comment

Remind me in a couple of days if I haven't returned to take a proper look; too late today.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/Building_targets.md Outdated Show resolved Hide resolved
doc/Building_targets.md Outdated Show resolved Hide resolved
doc/Building_targets.md Outdated Show resolved Hide resolved
doc/Building_targets.md Show resolved Hide resolved
doc/Building_targets.md Outdated Show resolved Hide resolved

@pazos pazos force-pushed the pazos:lean_readme branch 2 times, most recently from 2fc503f to af2d8c4 Aug 17, 2019

@@ -0,0 +1,199 @@
# Setting up a build environment for KOReader

These instructions are intended to build the emulator in Linux and MacOS. Windows users are suggested to develop in a [Linux VM](https://www.howtogeek.com/howto/11287/how-to-run-ubuntu-in-windows-7-with-vmware-player/) or using the [Windows Subsystem for Linux](https://en.wikipedia.org/wiki/Windows_Subsystem_for_Linux).

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 17, 2019

Member

WSL was more thinking aloud. I'm not sure we should recommend using WSL unless someone's actually verified it works for the purpose? The AppImage runs in WSL so logic dictates it should, but... :-)

Not that I've verified VMWare player personally or anything, but it's Ubuntu ergo it will work as expected.

Since I happen to be typing in Windows right now I suppose I'll give this a go, but it wants to restart after executing the first command and I don't want to restart. :-P

## Prerequisites

To get and compile the source you must have `patch`, `wget`, `unzip`, `git`,
`cmake` and `luarocks` installed, as well as a version of `autoconf`

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 17, 2019

Member

luarocks seems to have become a minor issue in the last year (regardless whether we're talking Mac OS or bleeding user distros like Arch & Gentoo). For Docker we roll out our own, see https://github.com/koreader/virdevenv/blob/9b41d9f36d9b20fdf9b58d04104cc9133fc15ce5/docker/ubuntu/baseimage/install_luarocks.sh#L7-L13

We should probably at least add a little note to that script for the moment, although perhaps ideally e'd move away from system luarocks since its config files/options and such are annoyingly variable across versions.

This comment has been minimized.

Copy link
@pazos

pazos Aug 17, 2019

Author Contributor

We can address this issue here too: #2798 (comment)

doc/Building.md Outdated Show resolved Hide resolved
doc/Building.md Outdated Show resolved Hide resolved
doc/Building.md Outdated Show resolved Hide resolved
doc/Building.md Outdated Show resolved Hide resolved
make KOR_BASE=../koreader-base
```

This will be handy if you are developing `koreader-base` and you want to test your

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 17, 2019

Member

Not sure I quite agree with this; just using branches in your base submodule works perfectly fine. But it's potentially slightly more convenient to quickly switch back & forth between different experiments, particularly if they involve Clang vs GCC or something drastic like that.

This comment has been minimized.

Copy link
@pazos

pazos Aug 17, 2019

Author Contributor

I never used that, but as you can imagine I copied blindly almost all the information and only tried to digest the doc for final users.

doc/Building.md Outdated Show resolved Hide resolved
doc/Building.md Outdated Show resolved Hide resolved
doc/Building.md Outdated Show resolved Hide resolved
doc/Building.md Outdated Show resolved Hide resolved
doc/Building.md Outdated Show resolved Hide resolved
@Frenzie
Copy link
Member

left a comment

Not sure if I used individual comments everywhere or if I left a few "review" comments, so submitting this review just in case.

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Not sure if I used individual comments everywhere or if I left a few "review" comments, so submitting this review just in case.

The idea of this PR was just move dev docs outside the main README.sh and provide links instead. I'm fine fixing (with your guide) things on devs documentation too. Feel free to commit specific suggestions as they're super useful to me. Please provide general feedback about the new README.sh after this PR: https://github.com/koreader/koreader/blob/b0d004df78172dc7f2aadf3a0bf9ecf17b1e84f2/README.md

Pinging @KenMaltby too for general feedback.

README.md Outdated Show resolved Hide resolved
@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Maybe you could link straight to the various install instructions instead of the somewhat roundabout redirect to the wiki front page? Besides that I'm pretty happy with it.

@pazos pazos added the documentation label Aug 17, 2019

pazos and others added 2 commits Aug 20, 2019
Update README.md
Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>

@pazos pazos force-pushed the pazos:lean_readme branch from 0eb2e20 to 3fef917 Aug 20, 2019

@Frenzie
Copy link
Member

left a comment

I figure that's it for the basic reorganization, I'd prefer to follow up on some of the points mentioned but we could merge this first.

@pazos

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I figure that's it for the basic reorganization, I'd prefer to follow up on some of the points mentioned but we could merge this first.

Yes, the things left to do are:

WSL
Docker
Luarocks
change KOReader-base suggestion

and none of them are in the README.

@Frenzie Frenzie merged commit 57d9f75 into koreader:master Aug 20, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Frenzie added a commit to Frenzie/koreader that referenced this pull request Aug 20, 2019
@Frenzie Frenzie referenced this pull request Aug 20, 2019
Frenzie added a commit that referenced this pull request Aug 20, 2019

@Frenzie Frenzie added this to the 2019.09 milestone Aug 22, 2019

@Frenzie Frenzie referenced this pull request Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.