Skip to content

Conversation

@haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented May 20, 2022

@haslinghuis haslinghuis self-assigned this May 20, 2022
@haslinghuis haslinghuis changed the title Update nwjs Update NWjs to 0.64 May 20, 2022
@github-actions

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented May 21, 2022

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> FAIL
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> PASS

@github-actions

This comment has been minimized.

McGiverGim
McGiverGim previously approved these changes May 23, 2022
@njdro
Copy link

njdro commented May 31, 2022

nwjs 0.63.x, 0.64.x, and 0.65.0 are producing faulty linux file rights from the packages. (i tested .deb).

0.62.2 is the last one to work.

The result is
image

due to group and others rights are set none:
image

instead of the expected read-only:
image

@haslinghuis
Copy link
Member Author

@meoso how about current version?

@njdro
Copy link

njdro commented May 31, 2022

@meoso how about current version?

master with nwjs 0.62.0 works fine. 0.63 breaks it

@haslinghuis
Copy link
Member Author

@meoso Thanks. Let's wait for a newer version instead.

https://nwjs.io/blog/

@McGiverGim
Copy link
Member

@haslinghuis I'm not too sure if we are a little off or I'm confusing some versions...

We use the .nvrmc file to define the Node version:


"engines": {
"node": "16.x"
},

But we ask for the 0.62 Node.js while building:

const nwBuilderOptions = {
version: '0.62.0',

According to the log, since 0.59.1 the Node.js version is v17.3.0: https://nwjs.io/blog/v0.59.1/

So, maybe I'm confusing something, but I think we are not using the correct vesion? Or the backwards compatibility is assured?

@haslinghuis
Copy link
Member Author

Agree we should solve it in tandem and should add Node.js v17.3.0 or perhaps even v18.3.0 if we add NWjs 0.65,1

See: https://github.com/nwjs/nw.js/blob/nw65/CHANGELOG.md

@haslinghuis haslinghuis changed the title Update NWjs to 0.64 Update NWjs to 0.65.1 and Node to 18.3.0 Jun 20, 2022
@haslinghuis
Copy link
Member Author

@meoso please check?

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@njdro
Copy link

njdro commented Jun 20, 2022

  • checked out 32e8edc
  • nvm install 18.3.0; nvm use 18.3.0; rm -rf node_modules ; yarn install
  • built release; installed release
  • user/executable rights do not apply correctly still for group & other
    image

Debian Linux 11
Linux 5.10.0-15-amd64 SMP Debian 5.10.120-1 (2022-06-09) x86_64 GNU/Linux

@github-actions

This comment has been minimized.

@haslinghuis haslinghuis changed the title Update NWjs to 0.65.1 and Node to 18.3.0 Update NWjs to 0.62.2 Jun 20, 2022
@haslinghuis haslinghuis force-pushed the update_nwjs branch 2 times, most recently from 9d4ecf9 to 42eb80e Compare June 20, 2022 21:17
@njdro
Copy link

njdro commented Jul 21, 2022

Seems so, but Debian being "stable" is always extremely slow to adopt as you can see talks started in 2018 and yet to be released. I suspect no fixes for many months or more. :(

@njdro
Copy link

njdro commented Jul 21, 2022

edited this comment twice now...
no "good" combinations are working for me. even if i build locally, the file rights problem have returned.
so the following problems exist:

  • NWjs versions that fix file-rights are out of sync with node LTS which this project wishes to adheres to.
  • zst compression fails for Debian

commits 711ea41and b14196e had worked with NWjs 0.62 & 0.65 , but unsure which node match is best 16.x or 18.x

Maybe Configurator's NWjs does not have to be in sync with blackbox-log-viewer's NWjs, as they both seem to have independent issues depending on versions used.

Sorry to spam the comments

@McGiverGim
Copy link
Member

I don't understand the problem exactly... some file in the new version uses some format that when decompressing is giving the error?
The problem is in the creation of the deb file or in the contents of it?

@njdro
Copy link

njdro commented Jul 26, 2022

I don't understand the problem exactly... some file in the new version uses some format that when decompressing is giving the error? The problem is in the creation of the deb file or in the contents of it?

The compression method used to compress the .deb file.
Results of /usr/bin/file on Betaflight's .deb files:
Old standard: Debian binary package (format 2.0), with control.tar.xz, data compression xz
Method in this PR: Debian binary package (format 2.0), with control.tar.zs, data compression zst which is not supported by Debian (or at least by Debian Stable 11)

@haslinghuis
Copy link
Member Author

@McGiverGim

Ubuntu has introduced zst compression in dpkg command. Debian does not have support for this protocol.

@McGiverGim
Copy link
Member

Then the problem is with the new version of Ubuntu used to build the deb file, not with this PR itself, right?

Then the only way to fix it is in this code:

return gulp.src([path.join(appDirectory, metadata.name, arch, '*')])
.pipe(deb({
package: metadata.name,
version: metadata.version,
section: 'base',
priority: 'optional',
architecture: getLinuxPackageArch('deb', arch),
maintainer: metadata.author,
description: metadata.description,
preinst: [`rm -rf ${LINUX_INSTALL_DIR}/${metadata.name}`],
postinst: [
`chown root:root ${LINUX_INSTALL_DIR}`,
`chown -R root:root ${LINUX_INSTALL_DIR}/${metadata.name}`,
`xdg-desktop-menu install ${LINUX_INSTALL_DIR}/${metadata.name}/${metadata.name}.desktop`,
],
prerm: [`xdg-desktop-menu uninstall ${metadata.name}.desktop`],
depends: ['libgconf-2-4', 'libatomic1'],
changelog: [],
_target: `${LINUX_INSTALL_DIR}/${metadata.name}`,
_out: RELEASE_DIR,
_copyright: 'assets/linux/copyright',
_clean: true,
}));

The dpkg-deb command has some parameter that we can pass to indicate the compression? It seems the -Z maybe? What will be the correct one?

@njdro
Copy link

njdro commented Jul 26, 2022

maybe yes. this may repair 1 of the 2 issues. (the other being file rights seemingly associated with older NWjs usage)

  -Z<type>                         Set the compression type used when building.
                                     Allowed types: gzip, xz, none.

unsure the source of the compression method change. im using ubuntu-latest in github-actions elsewhere and it does not introduce this issue by default, but i'm also on very old NWjs/node, so 🤷‍♂️

EDIT: gulp-deb node module is the source of the .deb build system. There is no parameter specification for changing compression method; however, -Zxz parameters for dpkg-deb command would be the correct fix for this particular issue.

@haslinghuis haslinghuis changed the title Update NWjs to 0.66.1 and Node to 16.16.0 LTS Update NWjs to 0.67.1 and Node to 16.17.0 LTS Aug 19, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the update_nwjs branch 2 times, most recently from 9f6eefe to 61a3696 Compare August 19, 2022 21:31
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!


function GUI_checkOperatingSystem() {
return navigator.userAgentData.platform;
return navigator?.userAgentData?.platform || 'Android';
Copy link
Member

Choose a reason for hiding this comment

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

I like it 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Navigator will be available always, but it does not hurt 😄

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Let's start the versions race again 😜
Let's merge this soon to see if Android works again

@haslinghuis haslinghuis merged commit 55d4506 into betaflight:master Aug 20, 2022
@haslinghuis haslinghuis deleted the update_nwjs branch August 20, 2022 22:31
@haslinghuis
Copy link
Member Author

@meoso we are on latest now. Did we forget something?

@njdro
Copy link

njdro commented Aug 22, 2022

@meoso we are on latest now. Did we forget something?

sorry i had to wait until today (Monday) to return to my Debian box.

 $ sudo dpkg -i betaflight-configurator_10.9.0-debug-7145cd6_amd64.deb 
dpkg-deb: error: archive 'betaflight-configurator_10.9.0-debug-7145cd6_amd64.deb' uses unknown compression for member 'control.tar.zst', giving up

i cannot test file attributes (chmod rights) until zst resolved.

Portable on Linux works fine.

@haslinghuis
Copy link
Member Author

@McGiverGim you have been contributing to gulp-debian library before. The project seems to have no activity as I can't find where I would put the -Z option. Maybe could be [hard]coded in the _exec('dpkg-deb --build ...) function in the library:

https://github.com/stpettersens/gulp-debian/blob/189c808db3d146fcbcaed62182a177bd58aa1b2a/index.js#L194

@McGiverGim
Copy link
Member

I did some change in the past. If the project is abandoned, there is some alternative?

@haslinghuis
Copy link
Member Author

haslinghuis commented Aug 23, 2022

@McGiverGim No alternatives I could find. Guess Debian users have to wait for zst implementation as Ubuntu pulled the merge trigger before consulting Debian on this feature. Localizing the library is not something I think is a maintainable solution (only short term)

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.

6 participants