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

Move ronn pages to lib #3997

Merged
merged 5 commits into from Oct 15, 2020
Merged

Move ronn pages to lib #3997

merged 5 commits into from Oct 15, 2020

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Oct 5, 2020

What was the end-user or developer problem that led to this PR?

The problem is that under the following conditions:

  • Bundler installed as a default gem by ruby-core installer.
  • Man executable not available.

Bundler would crash.

The problem is that if man is not available, bundler will fallback to displaying to .ronn plain text sources of the man pages. However, if bundler was installed as a default gem by ruby-core installer, those .ronn sources are not installed to RbConfig::CONFIG["mandir"].

What is your fix for the problem, implemented in this PR?

My fix, instead of artifically installing .ronn sources to man1 or man5 folders (which rubygems installer was currently doing), is to keep .ronn sources inside lib. That way, we can always find them no matter which type of bundler installation we're running.

The alternative was to change ruby-core installer to also install .ronn sources to RbConfig::CONFIG["mandir"] + "man1" and
RbConfig::CONFIG["mandir"] + "man5", but I found weird that we were doing that in the first place, since it seems to me that the man1 and man5 folders are only supposed to hold .1 and .5 files, respectively.

Fixes #3996.

Make sure he following tasks are checked

@bundlerbot bundlerbot added Bundler CI CI related issues RubyGems labels Oct 5, 2020
@deivid-rodriguez deivid-rodriguez force-pushed the better_man branch 2 times, most recently from 0d93254 to c51f6c9 Compare October 6, 2020 11:34
@hsbt hsbt self-requested a review October 6, 2020 11:52
It was overly complicated for no reason.
Since they will have different logic afterwards.
Only need to consider them for removing old versions.
@deivid-rodriguez
Copy link
Member Author

The alternative was to change ruby-core installer to also install .ronn sources to RbConfig::CONFIG["mandir"] + "man1" and
RbConfig::CONFIG["mandir"] + "man5", but I found weird that we were doing that in the first place, since it seems to me that the man1 and man5 folders are only supposed to hold .1 and .5 files, respectively.

The inverse counterargument could also be applied to lib: lib should only contain .rb files. However, we're already shipping template files inside lib, so for the shake of fixing this issue, I think it's fine to move ronn sources there.

@deivid-rodriguez
Copy link
Member Author

@hsbt Without having looked at the specific failures, due to the tests you had to revert in ruby/ruby#3659, I believe it's very likely that this PR will fix that problem.

Copy link
Member

@hsbt hsbt left a comment

Choose a reason for hiding this comment

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

Thanks, I checked the changes and test files. It seems fine to work for me.

@deivid-rodriguez deivid-rodriguez merged commit 59fa4e6 into master Oct 15, 2020
@deivid-rodriguez deivid-rodriguez deleted the better_man branch October 15, 2020 13:10
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Move ronn pages to lib

(cherry picked from commit 59fa4e6)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Move ronn pages to lib

(cherry picked from commit 59fa4e6)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Move ronn pages to lib

(cherry picked from commit 59fa4e6)
deivid-rodriguez added a commit that referenced this pull request Dec 7, 2020
Move ronn pages to lib

(cherry picked from commit 59fa4e6)
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.

Bundler does not find man pages (looks for txt version) when using ruby-head
3 participants