-
Notifications
You must be signed in to change notification settings - Fork 2.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
Remove deep-freeze-node
dependency: the project is no longer installable on mac arm
#13631
Comments
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 So when brave-browser goes looking for You can change it to That fixes it there, but then it breaks in brave-core which has its own You can then change at https://github.com/brave/brave-core/blob/master/build/commands/lib/config.js#L77 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. |
Fix is here https://github.com/brave/brave-browser/pull/14089/files, pending review. I didn't see anything else using that util.getNPMConfig and I was able to remove the string replacement and added a check if the src/brave folder already exists. |
hmm - I'm not running into any problems here on Windows. If you're seeing this, my first guess would be that you don't have Node LTS installed (we may have problems if you use latest version of Node.js) The folder is cc: @bridiver in case I missed something obvious |
FWIW, I ran into the same issue on a Mac Pro 10.15.7 (+ the issue of an installation failing requiring me to go and explicitly |
@Kwadz
then |
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. |
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
… On Feb 20, 2021, at 8:28 AM, Kwadz ***@***.***> wrote:
Thanks @bridiver, in that case I think this section of the documentation should be updated:
git clone ***@***.***:brave/brave-browser.git
cd brave-browser
npm install
# this takes 30-45 minutes to run
# the Chromium source is downloaded which has a large history
npm run init
It does not even match with this section:
# root where project is cloned
cd ~/brave-browser/
git remote add bsclifton ***@***.***:bsclifton/brave-browser.git
git fetch bsclifton
# root for the `brave-core` repo
cd src/brave
git remote add bsclifton ***@***.***:bsclifton/brave-core.git
git fetch bsclifton
I think the process in the documentation should be consistent and unified.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@Kwadz that part can look confusing, but it's correct. I believe I understand the disconnect Basically, all you need to do is run However- if you're actively making changes in a fork, you'll need to add YOUR remote to the By default, when running There may be some work you need to do if your fork's branch diverges significantly from upstream (origin) or if you have patches. In those cases you may need to edit the |
@bsclifton I reopened because there is still an outstanding issue with the npm config |
@Aragar199 I'm not able to reproduce the issue. Can you provide info on node, npm and OS version. #14089 breaks the install for me. |
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 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 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. |
@bridiver Thanks for following up on this. Before I submitted the PR to fix, I checked it on my Windows laptop as well and saw the same issue with the repo url showing up unidentified. However, I just checked the master branch on Mac and Windows and can no longer reproduce. I don't have anything to follow up on if @Kwadz is also no longer seeing an issue. |
@Kwadz there's only a different procedure (RE: branches) if you make edits to the code and would like to push those upstream. Everyone has read access to the repo, but creating branches on the remote and pushing to the remote is limited to employees. Thanks for the feedback though - definitely nice to have a fresh perspective on this 😄 |
I agree that all the setup instructions should be together so we’ll reorganize things a little bit
… On Feb 22, 2021, at 12:14 AM, Brian Clifton ***@***.***> wrote:
@Kwadz there's only a different procedure if you make edits to the code. Everyone has read access to the repo, but creating branches on the remote and pushing to the remote is limited to employees
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@bsclifton, you already explained it, thanks but this is not the point. The problem, IMHO, is the contributor guide is not clear in addition to be incomplete. I guess you didn't see what I wanted to point out. I think it's important for a project installation not to be difficult for new contributors. Anyway, I can create a specific issue about this, not to pollute this one with too much extrapolation 🙂 @Aragar199, about the Clone and initialize the repo section, I tested again the following:
I also tested with exactly the steps from the doc (without removing the brave dir):
|
Thank you for confirming @Kwadz I just checked and was able to reproduce again. Realized that it was skipping trying to clone in to the folder since it checks if the
|
@bridiver - |
@Aragar199 you're going to want to install the LTS version of Node.js, which should be 14.x |
Yep, that's it! I installed node 12 and it works fine. Still throws an error if the directory |
@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 |
node v12.20.2 |
yep, npm 6.x should work correctly because that is what I have locally |
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! |
This sounds like probably the best way to do it!
… On Oct 13, 2021, at 2:26 PM, Brian Johnson ***@***.***> wrote:
I use nvm which allows me to easily switch node versions. We are actively looking at ways to remove this local dependency.
> On Oct 13, 2021, at 2:06 PM, Daniel Houck ***@***.***> wrote:
>
>
> Yeah, I used the term “outdated” because just because Node supports it doesnʼt mean everyone who wants to build Brave does. Itʼs like saying Linux kernel 4.4, released in 2016, isnʼt outdated when the latest is 5.14. Sure, someone will still support you if you use it, but if what you do only works on it and not anything later, thatʼs a problem.
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#13631 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANNBA7U3V7P4JERFX6MXGALUGX2Q3ANCNFSM4WGP7UUA>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Hey I have followed the same instruction that @CatsMeow492 gave, but I am getting this error. I am using macbook air M1 ( macOS version 11.5.2) |
@himansh-gjr brew might not have arm builds for everything. I suggest trying |
@CatsMeow492 @bridiver yes I have tried this with But getting this error The process I did setup the projectforked and cloned the project
installed the required version of node and npm via nvm
Next
and here comes the error ! |
cc @petemill ^^ |
That package seems abandoned - we only seem to be using it in a couple tests where we can now use a different pattern. @himansh-gjr as a quick fix you could remove the |
deep-freeze-node
dependency: the project is no longer installable on mac arm
also @himansh-gjr just fyi we don't yet have official support for building on arm yet (arm builds are currently done on x64 just like for other platforms), but it sounds like @petemill may have it working soon. |
thanks @petemill but I think that quick fix does't seems to work |
Looks like an upstream error from gclient runhooks and might be intermittent. Did you try more than once?
… On Oct 21, 2021, at 6:31 PM, himansh-gjr ***@***.***> wrote:
thanks @petemill but I think that quick fix does't seems to work
still getting an error but this time a different one
My log file
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
I re-run |
I did everything from scratch again by forking both the repos
But then I still got this error, just not able to understand why this error occurred , is there any kind of pre- installation that I am missing ??
|
@himansh-gjr see brave/brave-core#10795 |
@himansh-gjr how is it looking now? |
@Aragar199 @bsclifton I still got the issue Windows 10 Education I removed the replace - with _ line in utils and have starting the init. |
For folks running into this issue with newer NPM - I created an issue and am tracking here (subscribe for updates): |
In that case, what is the correct way to go about trying to set up the project locally? |
I can't install the project. Steps to reproduce the issue:
And I get:
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).
The text was updated successfully, but these errors were encountered: