brave-browser: Remove `deep-freeze-node` dependency: the project is no longer installable on mac arm
I can’t install the project. Steps to reproduce the issue:
git clone git@github.com:brave/brave-browser.git
cd brave-browser
npm install
cd src
rm -rf brave # otherwise error "directory already exists", BTW that should be mentioned in the doc
cd ..
npm run init
And I get:
fatal: repository 'undefined' does not exist
It seems the function getNPMConfig relies on env vars that are obviously not present on my system. But there is no mention about it in the Brave installation procedure.
What’s wrong here?
Just in case, I mention that I’m on macOS 10.14.6 (Mojave).
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 52 (10 by maintainers)
I also encountered the same issue. Problem is that the path to the repository url is broken.
There is a string replacement that is making
-into_at https://github.com/brave/brave-browser/blob/master/lib/util.js#L5So when brave-browser goes looking for
brave-corein package.json, it can never find it because the key is changed tobrave_coreand it ends upundefinedYou can change it to
brave_corein https://github.com/brave/brave-browser/blob/master/package.json#L38That fixes it there, but then it breaks in brave-core which has its own
getNPMConfigthat does not do the same string replacement to change-. So it scans forbrave-corebut never finds it because it isbrave_corenow.You can then change at https://github.com/brave/brave-core/blob/master/build/commands/lib/config.js#L77
brave-coretobrave_coreso when it scans for the repo url it can find it because it is nowbrave_corein brave-browser/package.json.Then change the repo url and branch name to point to the brave-core version that has this change:
I was able to install using these steps, but it’s a patch job and should probably have a real fix instead.
I agree that all the setup instructions should be together so we’ll reorganize things a little bit
@jtrag if you are experiencing this problem you are using the wrong version of npm - https://github.com/brave/brave-browser/issues/13631#issuecomment-797611763
@Aragar199 you’re going to want to install the LTS version of Node.js, which should be 14.x LTS requirement captured here: https://github.com/brave/brave-browser/wiki/macOS-Development-Environment
@bsclifton I reopened because there is still an outstanding issue with the npm config
Tested with node v14.15.5 here (latest LTS), and it works. @bsclifton was right from the beginning and I also missed it 🤦♂️
Thank you guys!
yep, npm 6.x should work correctly because that is what I have locally
@bsclifton the problem is likely the npm version because I believe that is what converts the config to env vars, but presumably downgrading to node 14 will also take care of npm. There must be a change in the format when they are converted to env vars
Yep, that’s it! I installed node 12 and it works fine. Still throws an error if the directory
src/brave/already exists due to a failed installation but it’s much more verbose so I’m inclined to just nuke my PR and call it. My bad, @bsclifton, you mentioned this near the beginning of this thread!I think the documentation should be clarified.
You seem to tell the section Clone and initialize the repo is for Brave employees and the section Making change for contributors.
If there is a different procedure for Brave employees and a for contributors, that should be mentioned.
This
npm run initis not even mentioned in the section Making change. I think all the steps of the procedure should be mentioned in this section, or at least with a link to the proper section to build the project.My two cents about the doc:
For example in the section Clone and initialize the repo, “Clone and initialize the repo” for what? It’s not clear that part is not for contributing, may be we should mention something not to confuse with this section.
Also, the section Making change does not mention anything about
npm run init. So, it’s not complete and the reader think we should go to Clone and initialize the repo to continue the setup.I’d also update in Making change:
to
Hope my new eyes can help to clarify the doc and therefore make it easier for new contributors.
Not sure where that came from cc @bsclifton, I’ll check out the PR change and make sure all the docs are current either this weekend or first thing Monday
Thanks @bridiver, in that case I think this section of the documentation should be updated:
It does not even match with this section:
I think the process in the documentation should be consistent and unified.
Anyway, if this can be avoided it’s beneficial for everyone.