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

cinnamon.cjs: init at 4.4.0 #75680

Merged
merged 2 commits into from Jan 20, 2020
Merged

cinnamon.cjs: init at 4.4.0 #75680

merged 2 commits into from Jan 20, 2020

Conversation

mkg20001
Copy link
Member

Motivation for this change

Needed for porting the cinnamon desktop environment.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

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.

@jtojnar and I discussed how it's pretty terrible this needs an old spidermonkey version.
Let's not readd it globally. Please move that expression into the cjs directory and do

let 
  
  # https://github.com/linuxmint/cjs/issues/80
  spidermonkey_52 = callPackage ./spidermonkey_52.nix {};

in

pkgs/desktops/cinnamon/cjs/default.nix Show resolved Hide resolved
pkgs/desktops/cinnamon/cjs/default.nix Show resolved Hide resolved
pkgs/desktops/cinnamon/cjs/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/cjs/default.nix Outdated Show resolved Hide resolved
pkgs/desktops/cinnamon/cjs/default.nix Show resolved Hide resolved
@worldofpeace
Copy link
Contributor

Here's what's in cjs's .pc file btw

Requires: gobject-2.0 >= 2.42.0
Requires.private: gobject-introspection-1.0 libffi gthread-2.0 gio-2.0 >= 2.42.0 mozjs-52 cairo cairo-gobject gthread-2.0 gio-2.0 >= 2.42.0 mozjs-52 gtk+-3.0 >= 3.14.0

gobject-2.0 is in glib so we need to propagate that. The rest are private but they are linked against so they should be a buildInputs. I do believe, because this is a fork of gjs, it's pretty similar.

@mkg20001 mkg20001 force-pushed the pkg/cjs branch 2 times, most recently from 17967ac to 4d7d1d4 Compare December 15, 2019 04:50
@worldofpeace
Copy link
Contributor

#75680 (comment) and #75680 (comment) still stand.

I don't see references to some of those deps like upower https://github.com/linuxmint/cjs/search?q=upower&unscoped_q=upower. Or maybe the GitHub search isn't working.

@mkg20001 mkg20001 force-pushed the pkg/cjs branch 2 times, most recently from 7ea4540 to 042271c Compare December 30, 2019 00:46
@mkg20001
Copy link
Member Author

I've updated the dependencies to include those that are provided by the .pc file

I've left in the others, since otherwise I'd get some errors when trying to start the sessions.

Also, I'm using this on my lap so I try to not break it 😅

@worldofpeace worldofpeace changed the title cjs: init at 4.4.0 cinnamon.cjs: init at 4.4.0 Jan 8, 2020
Comment on lines 66 to 70
gnome3.caribou
keybinder3
upower
xapps
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these needed in CJS? I'm thinking maybe it's their typelib's in GI_TYPELIB_PATH are what's needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I suppose that's it

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I keep them? Is there a way to just add them into the typelib path?

Copy link
Contributor

Choose a reason for hiding this comment

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

What error does it produce when omitted?

Copy link
Member Author

Choose a reason for hiding this comment

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

When a .js file does things like require("xapps") it complains that it's typelib can't be found

Though I don't know if the typelibs should be added here or wrapped/added via env in another package

Copy link
Contributor

Choose a reason for hiding this comment

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

If the error occurs on importing xapps, the packages should be added to propagateBuildInputs of xapps and wrapGAppsHook should be added to the application importing xapps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not just xapps, all the others as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Well every runtime dependency that is not resolved at build time (e.g. hardcoded by a patch or a linker) needs to be propagated to be handled by a wrapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, should be done so far. All the apps use wrapGAppsHook

@mkg20001 mkg20001 force-pushed the pkg/cjs branch 3 times, most recently from 542b4a6 to e07eab2 Compare January 18, 2020 13:10
maintainers = [ maintainers.mkg20001 ];

knownVulnerabilities = [
"The runtime was extracted from Firefox 52, which EOL’d on September 5, 2018."
Copy link
Contributor

Choose a reason for hiding this comment

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

It should go to the spdermonkey package. Though I am not sure if it will be possible to whitelist it then.

Copy link
Member Author

Choose a reason for hiding this comment

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

The engine will only run code from cinnamon's source code or extensions, which are allowed to call commands anyways.
So we aren't lowering security by a lot by whitelisting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would whitelisting mean?

@mkg20001 mkg20001 force-pushed the pkg/cjs branch 3 times, most recently from e56a660 to fad6a28 Compare January 18, 2020 16:25
@worldofpeace
Copy link
Contributor

Built with

 nix-build -E 'with (import ./. { config = { permittedInsecurePackages = ["spidermonkey-52.9.0" ];}; }); cinnamon.cjs'

@worldofpeace
Copy link
Contributor

@jtojnar I've had @mkg20001 comment out the knownVulnerabilities because it doesn't appear hydra will build it. And that's going to be an issue.

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.

Should be good enough I suppose.

@worldofpeace worldofpeace merged commit 21b6279 into NixOS:master Jan 20, 2020
@@ -1,5 +1,6 @@
{ pkgs, lib }:

lib.makeScope pkgs.newScope (self: with self; {
xapps = callPackage ./xapps {};
cjs = callPackage ./cjs { };
Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like:

Suggested change
cjs = callPackage ./cjs { };
# This is insecure but we still want Hydra to build it.
unstafeCjsDoNotUse = callPackage ./cjs { };
cjs = unstafeCjsDoNotUse // {
meta = unstafeCjsDoNotUse.meta // {
knownVulnerabilities = [
"The SpiderMonkey runtime was extracted from Firefox 52, which EOL’d on September 5, 2018."
];
};
};

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that there are things that depend on cjs. As far as I understand it, we'd have to do this for every package that uses cjs as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants