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.

Sign up for GitHub

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

Jump to bottom

riot-desktop: init at 1.0.4 #57838

Merged
merged 1 commit into from Mar 23, 2019
Merged

riot-desktop: init at 1.0.4 #57838

merged 1 commit into from Mar 23, 2019

Conversation

pacien
Copy link
Contributor

@pacien pacien commented Mar 18, 2019 •
edited

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 riot-desktop-1.0.4 branch 2 times, most recently from 6273a4b to 03517c3 Compare March 18, 2019 15:17
@pacien
Copy link
Contributor Author

pacien commented Mar 18, 2019 •
edited

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 riot-desktop-1.0.4 branch 2 times, most recently from 7ae3629 to 6976eed Compare March 18, 2019 18:38
@pacien
Copy link
Contributor Author

pacien commented Mar 18, 2019 •
edited

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 March 18, 2019 18:51
@pacien pacien force-pushed the riot-desktop-1.0.4 branch 3 times, most recently from e12de61 to 81948f8 Compare March 18, 2019 19:35
bachp
bachp reviewed Mar 18, 2019
View reviewed changes
Copy link
Member

@bachp bachp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pacien
Copy link
Contributor Author

pacien commented Mar 18, 2019 •
edited

@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 riot-desktop-1.0.4 branch 2 times, most recently from 60f375c to e0126ad Compare March 21, 2019 18:27
@pacien
Copy link
Contributor Author

pacien commented Mar 22, 2019

I think this is ready to be merged.

worldofpeace
worldofpeace suggested changes Mar 22, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits that we should get.

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

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 Show resolved Hide resolved
pkgs/applications/networking/instant-messengers/riot/riot-desktop.nix Outdated

# executable wrapper
mkdir -p "$out/bin"
cat > "$out/bin/${executableName}" <<EOF
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add makeWrapper to nativeBuildInputs and do

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced.

pkgs/applications/networking/instant-messengers/riot/riot-desktop.nix Outdated Show resolved Hide resolved
@worldofpeace worldofpeace merged commit ec4af5c into NixOS:master Mar 23, 2019
@worldofpeace
Copy link
Contributor

@pacien Thanks a lot for your contribution ❇️

@pacien pacien mentioned this pull request Apr 15, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bachp bachp bachp left review comments

@aaronjanse aaronjanse aaronjanse left review comments

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pacien @worldofpeace @bachp @aaronjanse @GrahamcOfBorg