GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Update dependencies by Croydon · Pull Request #1519 · nodegit/nodegit

Conversation

@Croydon

@Croydon Croydon commented Jul 6, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Closes #1499
Closes #1237
Closes #1547
Closes #1534
Closes #1543
Closes #1490
Fixes #1539

I couldn't get #1365 & #1506 to work as these commits always froze Node 6 testing on Windows.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lenovouser, danielpassos, stefan-guggisberg, and zaucy reacted with thumbs up emoji lenovouser, tbranyen, mariecl, and zaucy reacted with heart emoji
All reactions
  • 👍 4 reactions
  • ❤️ 4 reactions
@Croydon
Croydon force-pushed the node10 branch from 0f72466 to 6a3b2d3 Compare July 11, 2018 00:11
@Croydon
Croydon force-pushed the node10 branch 2 times, most recently from 0df376a to d1fd091 Compare July 27, 2018 01:51
This was referenced Jul 27, 2018
Failed to compile on Node 10.0.0 #1490
Closed
NPM audit reveals 9 moderate vulnerabilities in dependencies #1499
Closed
@Croydon Croydon changed the title CI: Add Node.js 10 WIP: Add support for Node.js 10 Jul 27, 2018
@Croydon
Croydon force-pushed the node10 branch 13 times, most recently from cf000d5 to 00db706 Compare July 29, 2018 01:18
@Croydon Croydon changed the title WIP: Add support for Node.js 10 Update dependencies Jul 29, 2018
@Croydon
Croydon force-pushed the node10 branch from 00db706 to da25770 Compare July 29, 2018 01:45
@Croydon
Croydon force-pushed the node10 branch 2 times, most recently from 5c18164 to e31a4f4 Compare August 22, 2018 16:37
@Croydon

Croydon commented Aug 22, 2018

Copy link
Copy Markdown
Contributor Author

Node.js 6 on AppVeyor is exceeding the time limit of 1h and I'm not sure why. Has someone any idea?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Croydon

Croydon commented Sep 1, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Node 6 on Windows:

It seems to always freeze at the test step. At or after (not sure, likely after as the output indicates that the test is successful) the commit √ can get the commit diff test.

Can someone have a look who is more familiar with nodegit?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
implausible
implausible reviewed Sep 9, 2018
View reviewed changes
Comment thread .travis.yml Outdated
- master
- /^v\d+\.\d+\.\d+$/
sudo: true
dist: xenial

@implausible implausible Sep 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We shouldn't update to xenial until April of 2019. Ubuntu 14 is our minimum support until LTS ends.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tbranyen tbranyen Sep 19, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Really? Xenial seems to be pretty standard now. Is it possible that trusty would work and xenial would fail? Which would be more likely.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@implausible implausible Sep 20, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's more likely that xenial would work and trusty would fail. My main concern is that libcurl dependency for proxy support being dynamically linked.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Croydon
Croydon force-pushed the node10 branch 2 times, most recently from e56b722 to 7f4fd05 Compare September 19, 2018 23:06
@tbranyen

tbranyen commented Sep 19, 2018

Copy link
Copy Markdown
Member

This looks promising, I'll focus attention on this branch. How did you get the test error: Error: Callback is required and must be a Function. fixed?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Croydon
Croydon force-pushed the node10 branch from 2c5442e to 7f4fd05 Compare September 19, 2018 23:46
@Croydon
Croydon force-pushed the node10 branch 4 times, most recently from efc8fe6 to 9c7ef07 Compare September 28, 2018 18:57
@Croydon

Croydon commented Sep 28, 2018

Copy link
Copy Markdown
Contributor Author

Okay, after who knows how many rebases, commits and resets this should be merge ready.

@implausible @tbranyen
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@implausible

implausible commented Sep 28, 2018

Copy link
Copy Markdown
Member

The tests failed, so I just restarted them to see if it's some of that appveyor inconsistencies I've seen with compiling.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Croydon

Croydon commented Sep 28, 2018

Copy link
Copy Markdown
Contributor Author

Oh no. That seems like the same bug as CI had before at some points. Node 6 on Windows freezes in some test and never resumes.

Seems like some dependency update isn't compatible with Node 6 or brings another behaviour as on the newer Node versions.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Croydon
Croydon force-pushed the node10 branch from 59a9bd3 to 12a87da Compare September 28, 2018 22:53
@Croydon

Croydon commented Sep 29, 2018

Copy link
Copy Markdown
Contributor Author

So, the freezes are coming from the promisify-node commits (first two)...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Croydon Croydon mentioned this pull request Sep 29, 2018
Bump the promisify-node package #1506
Closed
@Croydon
Croydon force-pushed the node10 branch from 7719ed0 to afdb39a Compare September 29, 2018 14:47
@Croydon

Croydon commented Sep 29, 2018

Copy link
Copy Markdown
Contributor Author

I have ripped out the promisify-node updates, now CI should give green light
mariecl and emmax86 reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
implausible
implausible reviewed Oct 1, 2018
View reviewed changes
Comment thread appveyor.yml

branches:
only:
- master

@implausible implausible Oct 1, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but doesn't the removal of this line cause every branch on the nodegit repo to be built / tested on appveyor?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Croydon Croydon Oct 1, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, and in every fork with has activated AppVeyor.
Having the CI running only in pull requests in annoying.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@implausible implausible Oct 1, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think I am willing to approve this change. For instance, some maintainers, not just myself, will push their branches to the nodegit repo, and then make PRs against master. In those instances, we will be double building branches for no reason. It will also clutter up our appveyor / travis test results.

Is it too much to ask to create a pull request when you are looking to test against CI? I'm sorry that you feel that's too annoying!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@implausible implausible Oct 1, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

At best we could split this and the travis change out, and come back to it in a separate PR where we can have more discussion about it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Croydon added 8 commits October 2, 2018 01:32
@Croydon
Drop support for Node.js 4
3c38e7d 
as it is EOL
@Croydon
Update lodash to 4.17.11
933ae6f
@Croydon
Travis: Make a failure an actual failure
1f44e13
@Croydon
AppVeyor: Chores
bdb6942
@Croydon
AppVeyor: Build with newer Node versions first
4197240
@Croydon
Chores
6e5715c
@Croydon
Travis: Build downloads on Trusty
77aded7 
Dear future contributors, please update this in April 2019 when Trusty becomes EOL.

Keep extended testing and documentation upload on Xenial as it is easier there to install required dependencies and other nice perks.

Also don't allow Node 10 to fail anymore.
@Croydon
Update nan to 2.11.0
ee480af
@Croydon
Croydon force-pushed the node10 branch from afdb39a to ee480af Compare October 1, 2018 23:35
@Croydon

Croydon commented Oct 1, 2018

Copy link
Copy Markdown
Contributor Author

Removed commits which lets the Travis and AppVeyor run on all branches, even though I would strongly recommend it for the future.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
implausible
implausible reviewed Oct 2, 2018
View reviewed changes
Comment thread .travis.yml Resolved

Uh oh!

There was an error while loading. Please reload this page.
Croydon added 2 commits October 2, 2018 03:20
@Croydon
Chore: Package updates
99eccc9
@Croydon
Travis: Sudo is not needed
e42502f
@implausible

implausible commented Oct 2, 2018

Copy link
Copy Markdown
Member

Thanks @Croydon!
mariecl reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@implausible
implausible merged commit 722a3b6 into nodegit:master Oct 2, 2018
@Croydon
Croydon deleted the node10 branch October 2, 2018 17:43
@olivierlemasle olivierlemasle mentioned this pull request Dec 3, 2019
Make compatible with promisify-node 0.4.0 #1365
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbranyen tbranyen tbranyen left review comments
@implausible implausible implausible left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@Croydon @tbranyen @implausible
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.