-
Notifications
You must be signed in to change notification settings - Fork 722
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
Fix the OpenJDK11 build instructions for macOS #3989
Fix the OpenJDK11 build instructions for macOS #3989
Conversation
c087aab
to
98319a4
Compare
@babsingh - please can you double check my changes? Thanks! |
@@ -469,6 +479,13 @@ tar -xzf freemarker.tgz freemarker-2.3.8/lib/freemarker.jar --strip=2 | |||
rm -f freemarker.tgz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the above tar
cmd, --strip=2
should be changed to --strip-components=2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change.
@@ -492,13 +509,15 @@ When you have all the source files that you need, run the configure script, whic | |||
|
|||
``` | |||
bash configure --with-freemarker-jar=/<my_home_dir>/freemarker.jar \ | |||
--with-freetype=/usr/local/Cellar/freetype/2.9.1/ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--with-freetype
is not required by default. it should be included as an optional step similar to Non-compressed references support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, is freetype a definite pre-req? If not, in what situation would a user need to include the --with-freetype option? Answers will help me phrase the note. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, freetype
is a definite pre-req. --with-freetype
can be used in situations where bash configure
is unable to auto-detect freetype
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Note added.
The following dependencies can be installed by using [Homebrew](https://brew.sh/): | ||
|
||
- [autoconf 2.6.9](https://formulae.brew.sh/formula/autoconf) | ||
- [bash 4.4.23](https://formulae.brew.sh/formula/bash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think bash 4
is required for JDK11. There is no harm in using bash 4
. bash 4
can be optional. if bash 4
is used
After verification, Bash 4 is needed for ./get_source.sh
. The following steps should be mentioned:
Bash version 4 is required by the ./get_source.sh
script that you will use in step 2, which is installed to /usr/local/bin/bash
. To prevent problems during the build process, make Bash v4 your default shell by typing the following commands:
# Find the <CURRENT_SHELL> for <USERNAME>
dscl . -read <USERNAME> UserShell
# Change the shell to Bash version 4 for <USERNAME>
dscl . -change <USERNAME> UserShell <CURRENT_SHELL> /usr/local/bin/bash
# Verify that the shell has been changed
dscl . -read <USERNAME> UserShell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this.
e880a57
to
39a1c96
Compare
@babsingh - think I have addressed all your comments now. I also added in OpenSSL support now that this work is in place for OpenJDK 11. Probably should have done this in a separate PR, but hope you are ok with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the commit message has Add OpenSSL instructions since the associated work is complete
. this should also be included in the pull request's description. also, the title of the pull request and commit message should be updated to Fix the OpenJDK11 build instructions
since it is no longer macOS specific. otherwise, it looks good to me.
Add missing dependencies and configuration steps for macOS. Add OpenSSL instructions now that this work is also complete. [ci-skip] Closes: eclipse-openj9#3971 Signed-off-by: Sue Chaplain <sue_chaplain@uk.ibm.com>
39a1c96
to
ca1bb86
Compare
Yes, absolutely. Thanks @babsingh ... I've amended the commit message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Add missing dependencies and confirguration steps.
[ci-skip]
Closes: #3971
Signed-off-by: Sue Chaplain sue_chaplain@uk.ibm.com