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

feat: update ipfsd-ctl to 4.x #1411

Merged
merged 6 commits into from
Apr 23, 2020
Merged

feat: update ipfsd-ctl to 4.x #1411

merged 6 commits into from
Apr 23, 2020

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 17, 2020

Superseeds #1302. Closes #1412.

I'm starting this on a new branch with the version 3.x. I'm using a lot o @lidel's work on that PR.

@hacdias hacdias changed the title Feat/ipfsd ctl update feat: update ipfs controller Apr 17, 2020
@hacdias hacdias mentioned this pull request Apr 17, 2020
@hacdias hacdias changed the title feat: update ipfs controller feat: update ipfsd-ctl Apr 17, 2020
@hacdias hacdias changed the title feat: update ipfsd-ctl feat: update ipfsd-ctl to 3.x Apr 17, 2020
@hacdias
Copy link
Member Author

hacdias commented Apr 17, 2020

Seems to fail only on AppVeyor. It does seem to be something to do with the output parsing. For some reason I'm not being able to download Electron dep on Windows (too slow, networking issues?) right now.

Update: can reproduce errors (tests) on Windows!

@hacdias
Copy link
Member Author

hacdias commented Apr 17, 2020

@lidel it seems the tests failed on Windows because of some beautiful characters known as CR. .trim() seems to have solved the issue.

@hacdias hacdias requested a review from lidel April 17, 2020 15:20
@hacdias hacdias marked this pull request as ready for review April 17, 2020 15:21
@hacdias hacdias mentioned this pull request Apr 17, 2020
22 tasks
@lidel
Copy link
Member

lidel commented Apr 17, 2020

I'll take a look at this as soon as I figure out how to get tray icon back (#1153) 🙃

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

Just upgraded some files where I saw the API changed. Completely missed those!

@hacdias
Copy link
Member Author

hacdias commented Apr 20, 2020

Update to 4.x blocked: ipfs/js-ipfsd-ctl#501

@lidel
Copy link
Member

lidel commented Apr 21, 2020

@hacdias should be fixed in js-ipfsd-ctl v4.0.1 – mind taking a look?
(We need the fixed support for IPFS_PATH that comes with 4.x)

@lidel lidel mentioned this pull request Apr 21, 2020
2 tasks
@hacdias hacdias changed the title feat: update ipfsd-ctl to 3.x feat: update ipfsd-ctl to 4.x Apr 21, 2020
@hacdias
Copy link
Member Author

hacdias commented Apr 21, 2020

@lidel seems to be working!

hacdias and others added 5 commits April 22, 2020 08:58
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
This fixes a bug when local  ipfsd.stop() was called even when no
IPFS_PATH/config was present.

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Apr 22, 2020

Just rebased on top of master

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I did some smoke tests and looks ok.
@hacdias feel free to merge it (API improvements are really important, sooner we have them the better)

ps. found unrelated UX issue, but filled #1436 for that.

@@ -78,21 +78,19 @@ module.exports = async function (ctx) {
const log = logger.start('[ipfsd] stop daemon', { withAnalytics: 'DAEMON_STOP' })
updateStatus(STATUS.STOPPING_STARTED)

if (!fs.pathExists(join(ipfsd.repoPath, 'config'))) {
if (!fs.pathExistsSync(join(ipfsd.path, 'config'))) {
Copy link
Member

@lidel lidel Apr 17, 2020

Choose a reason for hiding this comment

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

@hacdias pathExists returns a promise, FYSA I fixed this (247b3a8) to use pathExistsSync instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Oopsie. I knew that but completely missed. Probably forgot adding an await. Thanks!

@lidel lidel mentioned this pull request Apr 22, 2020
1 task
@hacdias hacdias merged commit a32b295 into master Apr 23, 2020
@hacdias hacdias deleted the feat/ipfsd-ctl-update branch April 23, 2020 07:01
const freeGatewayPort = await portfinder.getPortPromise({ port: gatewayPort, stopPort: gatewayPort + 100 })
const freeApiPort = await portfinder.getPortPromise({ port: apiPort, stopPort: apiPort + 100 })
const findFreePort = async (port, from) => {
port = Math.max(port, from, 1024)
Copy link

@guanzo guanzo Feb 18, 2022

Choose a reason for hiding this comment

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

Hello, this Math.max call prevents me from setting a gateway port lower than 8080, and an api port lower than 5001. When I edit the config file and restart ipfs-desktop, it says the port is busy and suggests a new one.

For example if I try to set gateway port to 6437, then

const gatewayPort = 6437 // read from config file
const freeGatewayPort = await findFreePort(gatewayPort, 8080) // returns 8080

// evaluates to true, even though 6437 is free.
const busyGatewayPort = gatewayPort !== freeGatewayPort 

// I get a popup saying "The port 6437 is not available. Do you want to use 8080 instead?"

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

Successfully merging this pull request may close these issues.

3 participants