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

Enable CI platforms to build Linux, Windows and OS X releases #63

Merged
merged 104 commits into from
Apr 13, 2018

Conversation

JamesCropcho
Copy link
Contributor

Hello all,

This would enable the CI platforms to build a cross-platform set of releases:

These releases have not been tested on their respective platforms, nor are they necessarily in their final format. However, this pull would indeed enable some form of successful build for each of the three platforms we are targeting.

What do you all think?

―James

@JamesCropcho
Copy link
Contributor Author

To my knowledge, no issues raised are presently unaddressed by me, nor is "the ball in my court" regarding any discussion subthreads on this pull request.

@jackparmer jackparmer mentioned this pull request Apr 8, 2018
package.json Outdated
@@ -43,6 +60,7 @@
"plotly"
],
"dependencies": {
"ajv": "^6.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard Adding this dependency resolved this error: ajv-validator/ajv#708. If you like to learn more, you might start at npm/npm#19877. I admit I have only cursorily glanced at it. More importantly, I got the above AJV error, and I fixed it by adding this explicit dependency (it was presumably already an implicit dependency).

Hope that explains things. Let me know if you have additional questions or thoughts about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JamesCropcho but we don't use ajv-keyword, do we?

Copy link
Contributor Author

@JamesCropcho JamesCropcho Apr 11, 2018

Choose a reason for hiding this comment

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

I forgot about Étienne's comment: we should remove ajv from the deps.

Okay, I have done so, and the previous error is no longer present.

@etpinard
Copy link
Contributor

Looking good except for that docker-push (intermittent?) failure. Anyone knows what's that about?

@n-riesco
Copy link
Contributor

n-riesco commented Apr 11, 2018

@etpinard @JamesCropcho How about merging docker-push and docker-build into one job? Then, there wouldn't be a chance of getting that missing image error.

…here wouldn't be a chance of getting that missing image error."
@etpinard
Copy link
Contributor

@etpinard @JamesCropcho How about merging docker-push and docker-build into one job? Then, there wouldn't be a chance of getting that missing image error.

It would be best to double-check with the docker-push step author @scjody before making a change like this.

@JamesCropcho
Copy link
Contributor Author

How about merging docker-push and docker-build into one job?

Okay, I have done this.

@JamesCropcho
Copy link
Contributor Author

@etpinard @n-riesco

It would be best to double-check with the docker-push step author @scjody before making a change like this.

Just saw this. The commit may be easily reverted upon request.

@scjody
Copy link
Contributor

scjody commented Apr 11, 2018

The reason docker-push and docker-build are separate steps are that this significantly cuts down test time by allowing docker-build to run alongside the other tests.

Has anyone looked into the "missing image" error? It might be worth talking to CircleCI support if it happens consistently.

Also while I'm here, I know @etpinard isn't big on rebases but I strongly suggest cleaning up the commit history of this PR before merging. It's up to you folks though.

@etpinard
Copy link
Contributor

Also while I'm here, I know @etpinard isn't big on rebases but I strongly suggest cleaning up the commit history of this PR before merging. It's up to you folks though.

Yes, we should absolutely rebase these commits here.

@n-riesco
Copy link
Contributor

Also while I'm here, I know @etpinard isn't big on rebases but I strongly suggest cleaning up the commit history of this PR before merging. It's up to you folks though.

Squash and merge?

@JamesCropcho
Copy link
Contributor Author

@scjody @etpinard @n-riesco

Squash and merge?

I too believe an automated squash-and-merge would be the local best-of-all-worlds.

I am not taking additional action with regards to Docker workflow unless instructed, or asked questions about it.

@scjody
Copy link
Contributor

scjody commented Apr 12, 2018

My preference would be to rebase this down to 10 or so well thought out and understandable commits, which will increase the usefulness of our git history (especially on existing files) vs. squashing.

However I realize that may be a significant amount of work and I'm not strongly against squashing.

@etpinard
Copy link
Contributor

Repost #63 (comment)

Has anyone looked into the "missing image" error? It might be worth talking to CircleCI support if it happens consistently.

@JamesCropcho
Copy link
Contributor Author

@etpinard

Has anyone looked into the "missing image" error? It might be worth talking to CircleCI support if it happens consistently.

I personally have not. I can talk to CircleCI support about it upon request, although I don't believe I would be best-qualified to do so. Also, I would be loathe to make this task a blocking issue for pull request acceptance.

@etpinard
Copy link
Contributor

Ok. Having talking to @scjody , merging docker-build and docker-push into one job isn't that big of a deal for now. Before the merger our total CI time was around 6 minutes, after the merged it is more like 8 minutes. Those two extra minutes is hardly worth time and effort at this stage.

Issue logged in -> #68

@etpinard
Copy link
Contributor

Squashing and merging. Great job @JamesCropcho with special thanks to @n-riesco for the help!

@etpinard etpinard merged commit 433d619 into master Apr 13, 2018
@etpinard etpinard deleted the test-builds-across-travis-circle-appveyor branch April 13, 2018 20:40
@JamesCropcho
Copy link
Contributor Author

Thank you the kind words, @etpinard! I'm very happy to see this branch being merged into master.

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.

5 participants