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

fix(deps): update dependency app-builder-bin to v4.2.0 #7774

Closed
wants to merge 9 commits into from

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Sep 12, 2023

Mend Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
app-builder-bin 4.0.0 -> 4.2.0 age adoption passing confidence

Configuration

πŸ“… Schedule: Branch creation - "before 3am on the first day of the month" (UTC), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

β™» Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

πŸ”• Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

@renovate renovate bot added the renovate label Sep 12, 2023
@changeset-bot
Copy link

changeset-bot bot commented Sep 12, 2023

⚠️ No Changeset found

Latest commit: 6573840

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Sep 12, 2023

βœ… Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
πŸ”¨ Latest commit 6573840
πŸ” Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/65f3e86472caf900082380d3
😎 Deploy Preview https://deploy-preview-7774--car-park-attendant-cleat-11576.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@renovate renovate bot force-pushed the renovate/app-builder-bin-4.x branch from 5506951 to 6a501a1 Compare November 12, 2023 17:21
Copy link
Contributor Author

renovate bot commented Nov 12, 2023

Edited/Blocked Notification

Renovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR.

You can manually request rebase by checking the rebase/retry box above.

⚠ Warning: custom changes will be lost.

@mmaietta
Copy link
Collaborator

@develar seems there's still an issue with the latest app-builder-bin. I've added chmod to fix the executable permissions issue so that most tests are passing. It seems there's an issue resolving dependencies though with the latest app-builder-bin changes/version

  ● posix smart unpack

    ENOENT: no such file or directory, scandir '/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-2ACDjf/test-project-9/node_modules/are-we-there-yet/node_modules/core-util-is'

Please recommend how to best proceed πŸ™‚

Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@mmaietta
Copy link
Collaborator

mmaietta commented Feb 14, 2024

@develar friendly ping. πŸ™‚ I'm not sure how to proceed further here. Sounds like either something broke in app-builder and/or these are the follow-up action items develar/app-builder#84 (comment)
This also might be related: develar/app-builder#93

Can you confirm?

# Conflicts:
#	packages/builder-util/package.json
#	packages/builder-util/src/util.ts
Copy link
Contributor Author

renovate bot commented Mar 9, 2024

Renovate Ignore Notification

Because you closed this PR without merging, Renovate will ignore this update (4.2.0). You will get a PR once a newer version is released. To ignore this dependency forever, add it to the ignoreDeps array of your Renovate config.

If you accidentally closed this PR, or if you changed your mind: rename this PR to get a fresh replacement PR.

@renovate renovate bot deleted the renovate/app-builder-bin-4.x branch March 9, 2024 19:08
@beyondkmp
Copy link
Contributor

@mmaietta Why close this PR? After this fix(mmaietta/app-builder#2), I tested the integration of the new app-builder with electron-builder, and the same error still occurs.

I noticed that in electron-builder, the path is directly concatenated, using the node modules path + the name of the deps.

const filePath = dirPath + path.sep + name

This approach doesn't work for the foo package inside a workspace. It seems necessary for app-builder to return the real paths of all dependencies, and then for electron-builder to directly use these paths instead of concatenating them on its own.

@mmaietta mmaietta restored the renovate/app-builder-bin-4.x branch March 11, 2024 14:23
@mmaietta mmaietta reopened this Mar 11, 2024
@mmaietta
Copy link
Collaborator

Looks like it was autoclosed by the PR when it was auto-linked to it. Will reopen it

@Galkon
Copy link

Galkon commented Mar 11, 2024

It will be majorly appreciated to see this merged + released in electron-builder, thank you all!

@mmaietta
Copy link
Collaborator

mmaietta commented Mar 11, 2024

@Galkon I can't until app-builder has its issue fixed (and/or new release) and CI tests pass in this PR, until then I'm at an impasse.

@beyondkmp
Copy link
Contributor

@mmaietta When returning from node-dep-tree, directly include the deps' dir, so we can use this dir without needing to concatenate anymore. Do you see any problems with this solution?

including the deps' dir result

[{"dir":"/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-4RhJzh/test-project-1/node_modules","deps":[{"name":"ms","version":"2.0.0","dir":"/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-4RhJzh/test-project-1/node_modules/ms"}]},{"dir":"/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-4RhJzh/test-project-1/packages/test-app/node_modules","deps":[{"name":"foo","version":"1.0.0","dir":"/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-4RhJzh/test-project-1/packages/foo"},{"name":"ms","version":"2.1.1","dir":"/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-4RhJzh/test-project-1/packages/test-app/node_modules/ms"}]}

@mmaietta
Copy link
Collaborator

@beyondkmp is that an app-builder change or an electron-builder change?

Currently checking if I can get write access to app-builder to help unblock PRs on that package

@beyondkmp
Copy link
Contributor

@mmaietta
It looks like modifications are needed on both sides. The app-builder needs to output the specific dir for each dep, and electron-builder should directly use the dep's dir instead of concatenating.

@mmaietta
Copy link
Collaborator

Makes sense. Sounds like a breaking change for app-builder, so I'll prob need to get write access to the project first. Looks like it could also use some changeset/changelog+CI+publishing automation as well

@beyondkmp
Copy link
Contributor

The merging process of app-builder-bin is too slow, so I directly forked a version called app-builder-precompiled-bin and updated it in this MR(beyondkmp#2). I also made modifications to the electron-builder code. You can check it out when you have time

@mmaietta
Copy link
Collaborator

mmaietta commented Mar 14, 2024

I actually just got write-access to app-builder, just working on getting the right permissions to do CI/versioning/release automation now.

Nice work! Can you open up a PR from that branch to this repo? (with the new dependency) Curious how all the CI tests play out.


Update: Release automation is set up, now testing PR changes with v5.0.0-alpha.0. Now just need to get any of your changes pushed there and I can handle the rest here

@beyondkmp
Copy link
Contributor

beyondkmp commented Mar 15, 2024

Nice work! Can you open up a PR from that branch to this repo? (with the new dependency) Curious how all the CI tests play out.

I'm investigating the cause of the failure. It seems like some might be due to permission issues, preventing them from running.

@mmaietta
Copy link
Collaborator

If using pnpm to publish your binary for testing as a new dependency, you'll need to have this configuration for retaining the +x permissions on app-builder
https://github.com/develar/app-builder/blob/dc5dc6dd07cdb6528ce931bf1782cf613236f923/package.json#L30-L47

@beyondkmp
Copy link
Contributor

@mmaietta The ci tests are failed because mksquashfs's format is wrong.

 cannot execute  cause=fork/exec /home/runner/.cache/electron-builder/appimage/appimage-13.0.1/linux-x64/mksquashfs: exec format error

In appimage-13.0.1, the arch of mksquashfs in linux-x64 is arm64. Please help release a new version to fix it.

➜  linux-x64 file mksquashfs
mksquashfs: ELF 64-bit LSB pie executable, ARM aarch64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, for GNU/Linux 3.7.0, BuildID[sha1]=7669be255e4e48f9ec78d683dcc2f1578012fc94, not stripped

@mmaietta
Copy link
Collaborator

So I don't control/have access to electron-builder-binaries atm, and I'm not seeing any solid guidance on the repo to contribute new binaries. I'll try and revert 13.0.1 -> 13.0.0 for AppImage in app-builder-bin package and see if that resolves the issue

@renovate renovate bot deleted the renovate/app-builder-bin-4.x branch March 18, 2024 06:16
@mmaietta mmaietta restored the renovate/app-builder-bin-4.x branch March 18, 2024 06:30
@renovate renovate bot deleted the renovate/app-builder-bin-4.x branch March 18, 2024 06:30
@mmaietta
Copy link
Collaborator

Renovate bot keeps deleting the branch. Opened new PR #8139

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.

None yet

3 participants