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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Windows): Add windows support #903

Merged
merged 4 commits into from
Oct 4, 2016
Merged

Conversation

lgeiger
Copy link
Member

@lgeiger lgeiger commented Oct 4, 2016

Welcome windows devs! 馃帀

Since we don't have zmq-prebuilt for electron on windows (yet), this will rebuilt zmq from source. Windows devs will need Visual Studio 2013 and Python 2.

@@ -4,6 +4,7 @@
"description": ":notebook: the nteract notebook",
"main": "index.js",
"scripts": {
"postinstall": "node scripts/rebuild.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

Is postinstall the right place for this?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably fine. prepublish is a likely candidate, but I don't know the lifecycle/order for electron-packager. If it's working, lets go with it for now. I'm assuming this doesn't run within the dist copy - anyone doing an install is in dev mode.

@codecov-io
Copy link

Current coverage is 83.74% (diff: 100%)

Merging #903 into master will not change coverage

@@             master       #903   diff @@
==========================================
  Files            47         47          
  Lines          1267       1267          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1061       1061          
  Misses          206        206          
  Partials          0          0          

Powered by Codecov. Last update 9473d15...e9c84b6

@@ -34,6 +35,7 @@
"dist:linux32": "npm run dist -- --platform=linux --arch=ia32",
"dist:osx": "npm run dist -- --platform=darwin --arch=x64 --icon ./static/icon.icns --extend-info=./static/extend.plist --app-category-type=public.app-category.developer-tools --app-bundle-id=io.nteract.nteract",
"dist:osx:signed": "npm run dist:osx -- --osx-sign",
"dist:win": "npm run dist -- --platform=win32 --icon ./static/icon.ico",
Copy link
Member

Choose a reason for hiding this comment

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

Excellent

if (err) {
throw err;
}
});
Copy link
Member

Choose a reason for hiding this comment

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

While I'm a fan of this, what are we re-doing that's already done in electron-rebuild?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried electron-rebuild and it didn't work out of the box. Probably it was missing a --build-from-source flag somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like I ran into this issue: electron/rebuild#53

@rgbkrk rgbkrk merged commit 039c157 into nteract:master Oct 4, 2016
@lgeiger lgeiger deleted the windows branch October 4, 2016 21:34
@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

Do you think we're ready to pull the trigger on switching to zmq-static?

@lgeiger
Copy link
Member Author

lgeiger commented Oct 5, 2016

If we switch to zmq-static linux and mac devs would need to install the extra dependencies.

I'm OK with it, but it would be harder for new contributors. On the other side we could use the latest electron then.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

I summon @jdetle to the conversation since I know he both wants new Electron and is closer to the feeling that new contributors face.

@lgeiger
Copy link
Member Author

lgeiger commented Oct 5, 2016

And one would need Python 2 for node-gyp.

That said, 馃憤 for moving to zmq-static (though that pretty much means reverting this PR 馃槃 )

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

Since we're actually shipping builds, at the very least it means that trial users can just roll with nteract directly and not have to build from source.

My original hesitation to enforce all the build tooling setup was barriers to entry for those that are more on the frontend dev side, less astute with Jupyter.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

though that pretty much means reverting this PR

We still have to do the rebuild though right?

@lgeiger
Copy link
Member Author

lgeiger commented Oct 5, 2016

We still have to do the rebuild though right?

Yes that can be done with electron-rebuild then.

@jdetle
Copy link
Member

jdetle commented Oct 5, 2016

A wild @jdetle has appeared! I have no idea what the value of zmq-static is right now, but in terms of installing additional dependencies, I think we're fine. We can always post some sort of caveat, "If you are having trouble installing dependencies for nteract, you're always welcome to ask questions in our Slack." (Of course I volunteer to help anyone having issues.)

@lgeiger
Copy link
Member Author

lgeiger commented Oct 5, 2016

@jdetle zmq-static is pretty much zmq-prebuilt without prebuilts 馃槃

It would mean we could use the latest electron release at the cost of building zeromq ourself.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

Is there a reasonable way to make zmq-prebuilt rely on zmq-static?

@rgbkrk
Copy link
Member

rgbkrk commented Oct 5, 2016

I'm only asking that because of the node kernel I've been hacking on - I want to keep it relying on prebuild - it makes install so painless.

@lgeiger
Copy link
Member Author

lgeiger commented Oct 5, 2016

@rgbkrk If we find a way of running build.js on preinstall if no binaries are found it would even make zmq-static obsulet.

Definitely doable but I don't know how.

@lgeiger
Copy link
Member Author

lgeiger commented Oct 6, 2016

@rgbkrk It looks like the preinstall script can be included into the gyp file.

This means we could rebuild zmq-prebuilt. I'll try it tomorrow and make a PR if it works.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 6, 2016

I need some sort of excited emoji.

@lgeiger
Copy link
Member Author

lgeiger commented Oct 6, 2016

It works! 馃帀

I couldn't wait to try it 馃槈. The PR will come tomorrow.

@lock
Copy link

lock bot commented Apr 3, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Apr 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants