ProtonMail / WebClients

Monorepo hosting the proton web clients
GNU General Public License v3.0
4.53k stars 571 forks source link

Invalid package-lock.json file #158

Closed vladimiry closed 5 years ago

vladimiry commented 5 years ago

Describe the bug

Can't install locked dependencies by running npm ci.

To Reproduce Steps to reproduce the behavior:

  1. clean the npm cache folder
  2. git clone git@github.com:ProtonMail/WebClient.git && cd WebClient
  3. npm ci
  4. See error:
    npm ERR! code 128
    npm ERR! Command failed: git checkout fdcb732818fe377e89836edccd937204395969e5
    npm ERR! fatal: reference is not a tree: fdcb732818fe377e89836edccd937204395969e5

So apparently there is no fdcb732818fe377e89836edccd937204395969e5 commit anymore in github:ProtonMail/pt-formgenerator repository (probably force push happened):

    "pt-formgenerator": {
      "version": "github:ProtonMail/pt-formgenerator#fdcb732818fe377e89836edccd937204395969e5",
      "from": "github:ProtonMail/pt-formgenerator#feat/upgrade"
    },

Workaround

npm install will work but it updates the lock file and dependencies versions which means the dependencies are not locked.

dhoko commented 5 years ago

Indeed it's a bug, it should not try to get this version, it's a dev version (cf from).

You should have

  "pt-formgenerator": {                                                                      
    "version": "github:ProtonMail/pt-formgenerator#7269be46a950f1458513ce1c259c1f92f17c1057",
    "from": "github:ProtonMail/pt-formgenerator#semver:^1.4.0"                               
  },                                                                                         

inside the lock, with this it works.

Edit: fix available :)

vladimiry commented 5 years ago

Thanks for a fast fix, I confirm npm ci works now.

vladimiry commented 4 years ago

I have to reopen the issue since npm ci should work on all the projects.

Status:

vladimiry commented 4 years ago

@dhoko, can you reopen? I don't see the respective button.

dhoko commented 4 years ago

WebClient/tree/v4: lock file exists, npm ci test failed

:grimacing: I will update the file, we don't really maintain the lock for the v4 as it's a wip based on the latest version of v3 (without a tag for calendar, cntacts etc.) it's hard to maintain

vladimiry commented 4 years ago

Fair enough. But can I expect when v4 gets closer to the release state all the lock files will be valid and maintained? I still think the issue should be reopened at least for tracking purposes.

dhoko commented 4 years ago

Yeah I will generate a new one + add something to sync it more often. Voilà, on WebClient/tree/v4 it's :ok:

vladimiry commented 4 years ago

Voilà, on WebClient/tree/v4 it's ok

npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with npm install before continuing.

dhoko commented 4 years ago

@vladimiry voilà, sorry :grimacing: Indeed we changed a major inside the dependencies and we didn't update the lock.

dhoko commented 4 years ago

Btw, now to be able to install the v4 you need some variables inside the env/.env

PM_SETTINGS_GIT=git@github.com:ProtonMail/proton-mail-settings.git
CONTACTS_GIT=xxx
CALENDAR_GIT=xxxx

To inform the build script of the other applications.

vladimiry commented 4 years ago

Getting module missing error

ERROR in [copy-webpack-plugin] unable to locate 'node_modules/proton-translations' at '/home/vy/dev/_github/electron-mail/output/git/WebClient/4d751fc3ec902a33e14a81076292d14174047a6d/app.protonmail.ch/node_modules/proton-translations'

Should I remove { from: 'node_modules/proton-translations', to: 'i18n' }, record from CopyWebpackPlugin instance settings (monkey patching)?

dhoko commented 4 years ago

O_o you should not have an error. We create the directory when we install (as a hook postinstall) Do you see this warning when you run $ npm ci ?

Screenshot from 2020-02-19 09-57-11

Post npm ci I run npm run start:raw and It works :thinking:

Screenshot from 2020-02-19 09-59-02

I tried with a new copy, it worked too

git clone git@github.com:ProtonMail/WebClient.git --depth 1 --branch v4 dodo
cd dodo
npm ci
npm start

edit: same, no error with a new copy for the branch public and v4

vladimiry commented 4 years ago

@vladimiry voilà, sorry grimacing

Can confirm, it's in sync now, thanks.

Do you see this warning when you run $ npm ci ?

I do but the empty node_modules/proton-translations directory didn't get created, according to node_modules/proton-i18n/lib/postInstall.js.

PS I removed { from: 'node_modules/proton-translations', to: 'i18n' } from config by monkey patching the copy-webpack-plugin and build seems to work now.

dhoko commented 4 years ago

@vladimiry hum, weird. PS instead of changing the config, you can create the directory node_modules/proton-translation ;)

Can you give me some informations about your env so I can try to generate the same issue ? OS, node -v, npm -v etc.Thx

vladimiry commented 4 years ago

PS instead of changing the config, you can create the directory node_modules/proton-translation ;)

Yeah, that's a simpler workaround.

Can you give me some informations about your env so I can try to generate the same issue ?

I think no need since it works if I run stuff manually in console but doesn't create that dir in automated workflow enabled in https://github.com/vladimiry/ElectronMail.

vladimiry commented 4 years ago

Another build issue placed here https://github.com/ProtonMail/proton-calendar/issues/2

vladimiry commented 4 years ago

Btw, now to be able to install the v4 you need some variables inside the env/.env

I got it built for me without tweaking env/.env file. I see that checkEnv bash function is being executed only if I pass --deploy-subproject=-like arguments which I don't do.

Another build issue placed here ProtonMail/proton-calendar#2

Got the proton-calendar from my cache but fresh build is impossible at the moment.

dhoko commented 4 years ago

I think no need since it works if I run stuff manually in console but doesn't create that dir in automated workflow enabled in https://github.com/vladimiry/ElectronMail.

:thinking: where ? This is on a CI ? It's an issue if you don't have the empty directory, proton-i18n should create an empty one.

I fixed the calendar etc. It was out-of-date :/

vladimiry commented 4 years ago

thinking where ? This is on a CI ? It's an issue if you don't have the empty directory, proton-i18n should create an empty one.

It got worked around for now, here https://github.com/vladimiry/ElectronMail/blob/5494933871bac7b7aea48fd999447007c100cdb0/scripts/prepare-webclient/protonmail.ts#L112-L114

dhoko commented 4 years ago

hahaha :rofl: that's a solution. But i would like to find a permanent solution via the hook when we install. We don't have the issue here :/

vladimiry commented 4 years ago

But i would like to find a permanent solution

Mee to, so I put TODO there. You know, if dev puts TODO somewhere it will be handled, one day ... :laughing:

vladimiry commented 4 years ago

@dhoko can you also sync lock file in proton-mail-settings? It used to be synced but not anymore.

dhoko commented 4 years ago

@vladimiry :heavy_check_mark: voilà

dhoko commented 4 years ago

@vladimiry I created a patch for your issue https://github.com/ProtonMail/proton-i18n/pull/13 you install via npm ci, so the fix will be available with the new version. :)