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

Issue 19716 Add MANPATH to activate(.fish) #367

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bolry
Copy link

@bolry bolry commented Mar 3, 2019

Add the MANPATH environment variable to both the activate and activate.fish generated scripts in install.sh. Gives access to man pages for, e.g., dmd and dmd.conf.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @bolry! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
19716 enhancement activate and activate.fish scripts do not add MANPATH

@wilzbach
Copy link
Member

wilzbach commented Mar 4, 2019

Thanks a lot for your PR, but could you please add a simple test for it?
Check travis.sh for details.

@bolry
Copy link
Author

bolry commented Mar 4, 2019

Will do. Travis is new to me but I'll have a look.

@wilzbach
Copy link
Member

wilzbach commented Mar 4, 2019

It's basically just one big shell script which we run in the CI. Let me know if you need help.

@bolry
Copy link
Author

bolry commented Mar 4, 2019

Seemingly I need some ideas about the travis runtime environment to check it. Maybe man isn't available?

@wilzbach
Copy link
Member

wilzbach commented Mar 4, 2019

I presume LDC just doesn't come with a man page or has it somewhere else?

travis.sh Outdated Show resolved Hide resolved
@bolry
Copy link
Author

bolry commented Mar 13, 2019

I've changed to only add MANPATH for dmd compilers.

@AndrewEdwards
Copy link
Contributor

@bolry Request review and resolution of any conflicts associated with this pull request. Let's whip this in shape for merger.

@@ -710,17 +714,19 @@ write_env_vars() {
esac

logV "Writing environment variables to $ROOT/$1/activate"
cat > "$ROOT/$1/activate" <<EOF
cat > "$ROOT/$1/activate" <<END_ACTIVATE
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? EOF is kinda a standard for bash heredoc strings.

# Check man pages do not destroy system man pages availability
if [[ $compiler = dmd* ]]; then
. ~/dlang/$compiler/activate
man dmd >/dev/null 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

Maybe grep for sth. like Digital Mars to ensure it worked properly?

@@ -75,6 +75,13 @@ do
command -v dub >/dev/null 2>&1 || { echo >&2 "DUB hasn't been installed."; exit 1; }
deactivate

# Check man pages do not destroy system man pages availability
Copy link
Member

Choose a reason for hiding this comment

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

You don't check this ;-)

@AndrewEdwards
Copy link
Contributor

@wilzbach, I doubt @bolry is with us anymore. Do you mind taking over and seeing this to completion?

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.

4 participants