-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Improve ccache instructions Fixes #4215 #4218
Conversation
✅ Deploy Preview for react-native ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Great job! thanks for updating the website.
I left a few nits for some unrelated changes. Once you revert these files, we can merge this!
website/src/css/customTheme.scss
Outdated
@@ -585,14 +585,15 @@ article header h1 { | |||
line-height: 3rem; | |||
} | |||
|
|||
article header h2, { | |||
article header h2 { |
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.
nit: these changes should be reverted. They are unrelated to the changes we want to push here! 😉
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.
@cipolleschi, I added the stylesheet changes to a separate commit.
Someone should do them at some stage since prettier hasn't been run.
Would you like for me to add them to a separate PR?
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.
yeah, that would be great. Also, prettier is running in CI, theoretically... 🤔
website/src/css/customTheme.scss
Outdated
line-height: 3rem; | ||
|
||
a { | ||
color: var(--ifm-font-color-base); | ||
font-size: 2.5rem; | ||
|
||
&:hover, &:focus { | ||
&:hover, |
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.
same as above
@@ -72,7 +72,8 @@ | |||
svg[class^="authorSocialLink"] { | |||
fill: var(--subtle) !important; | |||
|
|||
&:hover, &:focus { | |||
&:hover, |
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.
same as above
@@ -8,7 +8,10 @@ import { | |||
useDocVersionSuggestions, | |||
} from '@docusaurus/plugin-content-docs/client'; | |||
import {ThemeClassNames} from '@docusaurus/theme-common'; | |||
import {useDocsVersion, useDocsPreferredVersion} from '@docusaurus/plugin-content-docs/client'; | |||
import { |
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.
same as above
@cipolleschi OK prettier commit removed. I'll open another PR shortly |
``` | ||
ln -s $(which ccache) /usr/local/bin/gcc | ||
ln -s $(which ccache) /usr/local/bin/g++ | ||
ln -s $(which ccache) /usr/local/bin/cc | ||
ln -s $(which ccache) /usr/local/bin/c++ | ||
ln -s $(which ccache) /usr/local/bin/clang | ||
ln -s $(which ccache) /usr/local/bin/clang++ | ||
``` |
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.
Those instructions are needed for Android, why are you removing them?
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.
Ahh, I was focussed on the iOS use cache and given the new script calls ccache directly I assumed they weren't needed.
I can create another PR to put it back. However is it definitely needed. I noticed this in the cmake files
https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/jni/CMakeLists.txt#L16-L21
https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/jni/CMakeLists.txt#L16-L21
I can do some testing?
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 we would have to verify if it's still needed or not
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.
Hmm so I can't even get ccache to work on android.
From my testing the ndk compiler gets used
/home/johnf/opt/Android/SDK/ndk/26.1.10909125/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++
In RN 0.74 support was added to natively support ccachng on ios by setting
enable_ccache
in the Podfile.This change updates the relevant docs to use the much simpler instructions