-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(linux): make semver pre-release versions valid for "pacman" and "rpm" target #7630
fix(linux): make semver pre-release versions valid for "pacman" and "rpm" target #7630
Conversation
🦋 Changeset detectedLatest commit: 316de39 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for car-park-attendant-cleat-11576 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thanks @m4rch3n1ng for the PR! Quick Q, is the |
good call actually @mmaietta, did some quick research.
anyways that's what i found do tell me how (/ where) you want me to solve this, because a ternary is probably not the best idea lol |
Great investigative work! Can you check electron-builder/packages/app-builder-lib/src/linuxPackager.ts Lines 50 to 57 in 8ae9061
|
alpine seems to be a lot more annoying than i thought and i literally cannot find any proper specs on their version format apart from this build script i also need to figure out how flatpak does it which is currently annoying me because i can't get it to build for me (it just gives me a my concern with a ternary would be that it would have to be nested
which is ugly and hard to read, so i thought about perhaps doing it in a seperate function with a switch statement? but i wasn't sure where |
tested some more with alpine and surprisingly it seems to. just work like that? i don't honestly know. can't execute the file because i do not have a gui / display server here and i have no idea what would happen if you were to want to publish it, though to be fair i think it's fair to assume you don't really want to publish your pre-release versions, but it installed correctly as |
super unsure on what to do with solaris and flatpak, but i think it should be fine to just fix it for rpm and pacman for now perhaps. if it's just those two i could actually just use a ternary i guess, tho it's still not ideal. |
Fantastic debugging work. Thank you Okay, let's pull a helper function to
In
|
done |
Many thanks for your contribution! |
fix #7601
this replaces all hyphens (
-
) in versions when targeting"pacman"
with underscores (_
) to follow the pacman version format.notes:
i am only replacing the version of the package, not of the
.pacman
file, since doing so would (i think) require overwriting the version inappInfo.version
, which is marked as readonly,electron-builder/packages/app-builder-lib/src/appInfo.ts
Line 24 in 285aa76
or adding a seperate rule to the
macroExpander
, which, as far as i can tell, does not have access to the current build target (which could be solved fairly easily with a new optional parameter tho)electron-builder/packages/app-builder-lib/src/util/macroExpander.ts
Line 4 in 285aa76
neither of which seemed like a good idea to me, especially without prior feedback.
it also doesn't matter for
pacman
and looks more consistent with the other installer files, if unchanged.i am using
.replace(/-/g, "_")
over.replaceAll("-", "_")
since the latter was only added in node v15.0.0 and this package still supports node v14i have a very limited idea on how this codebase is structured so tell me if i should do something differently