Skip to content
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

riot-desktop: init at 1.0.4 #57838

Merged
merged 1 commit into from Mar 23, 2019

Conversation

Projects
None yet
5 participants
@pacien
Copy link
Contributor

commented Mar 18, 2019

Motivation for this change

This introduces a package for the Electron version of the Riot instant messaging client.

This PR is heavily inspired by @Ralith's https://github.com/Ralith/riot-electron-nix.

Upstream is switching from npm to yarn, so I attempted to use yarn2nix to obtain a nix expression for all the dependencies.

Yarn seems to complain about not being able to download dependencies even though they are present in the freshly-built offline cache. I'm not sure whether I've hit a bug in Yarn. It was a yarn2nix version mismatch.

New problems have been unlocked (see comments).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@pacien pacien force-pushed the pacien:riot-desktop-1.0.4 branch 2 times, most recently from 6273a4b to 03517c3 Mar 18, 2019

@pacien

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Solved. Not relevant anymore.

❯ nix-build -E 'with import <nixpkgs> { }; callPackage ./riot-desktop.nix { }'
these derivations will be built:
  /nix/store/0fd7zfkpw7bnajrwiy38v4k6g528b3l2-riot-desktop.drv
building '/nix/store/0fd7zfkpw7bnajrwiy38v4k6g528b3l2-riot-desktop.drv'...
unpacking sources
unpacking source archive /nix/store/2i6vn4zh02d1075lvhrhbg5fz4cmkw4q-source
source root is source
patching sources
configuring
building
yarn run v1.9.4
warning You don't appear to have an internet connection. Try the --offline flag to use the cache for registry queries.
warning Skipping preferred cache folder "/homeless-shelter/.cache/yarn" because it is not writable.
warning Selected the next writable cache folder in the list, will be "/build/.yarn-cache-1000".
$ yarn build:js-sdk && yarn build:react-sdk && yarn reskindex && yarn build:res && yarn build:bundle --offline --frozen-lockfile --ignore-engines --ignore-scripts
warning Cannot find a suitable global folder. Tried these: "/usr/local, /homeless-shelter/.yarn"
warning You don't appear to have an internet connection. Try the --offline flag to use the cache for registry queries.
warning Skipping preferred cache folder "/homeless-shelter/.cache/yarn" because it is not writable.
warning Selected the next writable cache folder in the list, will be "/build/.yarn-cache-1000".
$ node scripts/yarn-sub.js matrix-js-sdk start:init
warning Cannot find a suitable global folder. Tried these: "/usr/local, /homeless-shelter/.yarn"
warning You don't appear to have an internet connection. Try the --offline flag to use the cache for registry queries.
warning Skipping preferred cache folder "/homeless-shelter/.cache/yarn" because it is not writable.
warning Selected the next writable cache folder in the list, will be "/build/.yarn-cache-1000".
$ babel -s -d lib src
warning Cannot find a suitable global folder. Tried these: "/usr/local, /homeless-shelter/.yarn"
Error: EACCES: permission denied, open 'lib/ReEmitter.js.map'
    at Object.fs.openSync (fs.js:646:18)
    at fs.writeFileSync (fs.js:1299:33)
    at outputFileSync (/nix/store/x37l0acmjqws5ycixk4fvdrvgwq3vg11-riot-web-modules-1.0.3/node_modules/output-file-sync/index.js:45:3)
    at write (/nix/store/x37l0acmjqws5ycixk4fvdrvgwq3vg11-riot-web-modules-1.0.3/node_modules/babel-cli/lib/babel/dir.js:30:7)
    at handleFile (/nix/store/x37l0acmjqws5ycixk4fvdrvgwq3vg11-riot-web-modules-1.0.3/node_modules/babel-cli/lib/babel/dir.js:43:7)
    at /nix/store/x37l0acmjqws5ycixk4fvdrvgwq3vg11-riot-web-modules-1.0.3/node_modules/babel-cli/lib/babel/dir.js:61:9
    at Array.forEach (<anonymous>)
    at handle (/nix/store/x37l0acmjqws5ycixk4fvdrvgwq3vg11-riot-web-modules-1.0.3/node_modules/babel-cli/lib/babel/dir.js:59:29)
    at Array.forEach (<anonymous>)
    at module.exports (/nix/store/x37l0acmjqws5ycixk4fvdrvgwq3vg11-riot-web-modules-1.0.3/node_modules/babel-cli/lib/babel/dir.js:69:15)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
child_process.js:650
    throw err;
    ^

Error: Command failed: yarn start:init
    at checkExecSyncError (child_process.js:607:13)
    at Object.execSync (child_process.js:647:13)
    at Object.<anonymous> (/build/source/scripts/yarn-sub.js:18:15)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:694:10)
    at startup (bootstrap_node.js:204:16)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
builder for '/nix/store/0fd7zfkpw7bnajrwiy38v4k6g528b3l2-riot-desktop.drv' failed with exit code 1
error: build of '/nix/store/0fd7zfkpw7bnajrwiy38v4k6g528b3l2-riot-desktop.drv' failed

From #riot:matrix.org:

Dave Official Matrix.org Team:
Notkea: so riot's build script does unconventional things in that it builds js-sdk and react-sdk too
so that when you're developin those projects stay up to date
and unfortunately I suspect that's biting you here because the nixos package build thingy is, very reasonably, not letting it write over a file from another package
so I suspect if you ran the last 3 steps of 'build' (ie. yarn reskindex && yarn build:res && yarn build:bundle ) it would work
although obviously be depending on details of the build system and therefore prone to breaking when we change stuff
also caution: it's possible we might change bit of the build process like this in the not-too-distant future

@pacien pacien force-pushed the pacien:riot-desktop-1.0.4 branch 2 times, most recently from 7ae3629 to 6976eed Mar 18, 2019

@pacien

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Solved. Not relevant anymore.

The evaluation seems to fail on ofborg: https://gist.github.com/GrahamcOfBorg/4198d18d361f0190654ab47e50e7ac3f

cannot read '/nix/store/i83lc95gk9bdmb7lmf1rfym2yhbr99bf-source/electron_app/package.json', since path '/nix/store/m4qpz1ivkbirdi34smph8jf4m0mldllm-source.drv' is not valid, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-0-gleber.ewr1.nix.ci/lib/trivial.nix:245:24

It seems to work fine on my machine using nix-build -E 'with import <nixpkgs> { }; callPackage ./riot-desktop.nix { }'.

@pacien pacien changed the title [WIP] riot-desktop: init at 1.0.4 riot-desktop: init at 1.0.4 Mar 18, 2019

@pacien pacien marked this pull request as ready for review Mar 18, 2019

@pacien pacien force-pushed the pacien:riot-desktop-1.0.4 branch 3 times, most recently from e12de61 to 81948f8 Mar 18, 2019

@bachp
Copy link
Contributor

left a comment

Would it be possible to share more with the riot-web package?

@pacien

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@bachp said:

Would it be possible to share more with the riot-web package?

The riot-desktop package is already based on the riot-web expression and only builds what's necessary to wrap it. I don't think it can be factorised further.

@pacien pacien force-pushed the pacien:riot-desktop-1.0.4 branch 2 times, most recently from 60f375c to e0126ad Mar 19, 2019

@pacien

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

I think this is ready to be merged.

@worldofpeace
Copy link
Member

left a comment

A few nits that we should get.

I have installed and launched this successfully btw.
Screenshot from 2019-03-22 14 14 36

Show resolved Hide resolved pkgs/applications/networking/instant-messengers/riot/riot-desktop.nix Outdated
Show resolved Hide resolved pkgs/applications/networking/instant-messengers/riot/riot-desktop.nix
Show resolved Hide resolved pkgs/applications/networking/instant-messengers/riot/riot-desktop.nix
Show resolved Hide resolved pkgs/applications/networking/instant-messengers/riot/riot-desktop.nix
# executable wrapper
mkdir -p "$out/bin"
cat > "$out/bin/${executableName}" <<EOF

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Mar 22, 2019

Member

You can add makeWrapper to nativeBuildInputs and do

makeWrapper ${electron}/bin/electron $out/bin/${pname} \
      --add-flags $out/share/riot/electron

This comment has been minimized.

Copy link
@pacien

pacien Mar 22, 2019

Author Contributor

Replaced.

Show resolved Hide resolved pkgs/applications/networking/instant-messengers/riot/riot-desktop.nix Outdated

@pacien pacien force-pushed the pacien:riot-desktop-1.0.4 branch from e0126ad to 839c988 Mar 22, 2019

@worldofpeace worldofpeace merged commit ec4af5c into NixOS:master Mar 23, 2019

12 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
riot-desktop on aarch64-linux Success
Details
riot-desktop on x86_64-linux Success
Details
@worldofpeace

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

@pacien Thanks a lot for your contribution ❇️

@pacien pacien referenced this pull request Apr 15, 2019

Merged

riot-{web,desktop}: 1.0.{6,4} -> 1.0.7 #59604

4 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.