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

Configure Project for Electron-Builder #52

Merged
merged 5 commits into from
Feb 16, 2018

Conversation

JamesCropcho
Copy link
Contributor

Hello all,

Note that this pull request is not necessarily a "request to pull," but rather serves also to demonstrate my actions thus far, and to facilitate discussion.

This PR grants the building of native Electron apps for *nix, Windows and OS X targets, via

$ electron-builder . -wml

and similar.

This PR grants the ability to use the Python Graph Exporter shell tool, via (example, *nix version)

$ cat ../mocks/5.json | ./dist/image-exporter-0.0.1-x86_64.AppImage > mock-05.png.

The AppImage target is a standalone executable, ~50MB.

All test pass.

Using /build the way it was done, is no longer possible with standard (even non-compiled) Electron-Builder settings, so I changed all references to point to a cross-platform tempdir.

―James

NB: I know I was given access to the blank https://github.com/plotly/python-image-exporter with the goal of keeping packaging concerns separate from feature concerns, however that has not proven to be possible (yet?) due to the tempdir architectural concern.

test/common.js Outdated

const paths = {}
const urls = {}
const mocks = {}

paths.root = path.join(__dirname, '..')
paths.build = path.join(paths.root, 'build')
paths.build = path.join(os.tmpdir(), 'plotly-graph-exporter-build')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep build/ for testing stuff. This will help debug on CI by more easily inspecting test artefacts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, if you would like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do this next week, unless otherwise requested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Next week sounds good. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I should have this fixed tomorrow!

@@ -22,7 +22,7 @@ tap.test('pdftops.pdf2eps', t => {
if (err) t.fail(err)

const size = fs.statSync(outPath).size
t.ok(size > 1e5, 'min pdf file size')
t.ok(size > 9e4, 'min pdf file size')
Copy link
Contributor

Choose a reason for hiding this comment

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

This resolves #48 I presume? Nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yessir.

"mac": {
"target": [{
"target": "default"
}]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding Linux deb target so that I can test it on my laptop? (Sorry for the inconvenience if any),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have Ubuntu as the main OS on my laptop, which is a Debian distribution. AppImage is the default format for this build target. If one runs electron-builder . or similar, one would get an AppImage which one can execute on Debian.

If you require me to address this further, let me know.

@etpinard
Copy link
Contributor

etpinard commented Feb 7, 2018

Excellent use of os.tmpdir which is way better than my relative-to-repo build/ attempt.

Great contribution 👌

I made one blocking comment: where I think we should keep build/ for test artefacts.

I can take care (and probably should 😉 ) of other one about making an Debian built myself.

@JamesCropcho
Copy link
Contributor Author

Thank you for the kind words, @etpinard.

…help debug on CI by more easily inspecting test artefacts."
@JamesCropcho
Copy link
Contributor Author

Hello, all!

Last we discussed this pull request, @etpinard wrote:

I made one blocking comment: where I think we should keep build/ for test artefacts.

I have just pushed a commit which does just that. Shall we now merge this branch?

@etpinard
Copy link
Contributor

etpinard commented Feb 15, 2018

Great! This PR is a big win 💃 (that means good to merge in plotly lingo that predates github PR review)

Would you opening up an issue about trying to build this thing for Linux, so that we don't forget about #52 (comment)

@JamesCropcho JamesCropcho merged commit 61fa5ed into master Feb 16, 2018
@JamesCropcho JamesCropcho deleted the configure_project_for_electron_builder branch February 16, 2018 21:24
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.

2 participants