-
-
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: use buildVersion
not buildNumber
for fpm --iteration
flag
#7075
fix: use buildVersion
not buildNumber
for fpm --iteration
flag
#7075
Conversation
…lectron-userland#6945) `buildNumber` is not overridable in the config and is provided directly by the CI, this should be `buildVersion` instead. Dashes are not supported for iteration in some versions of fpm, so replace with underscores. jordansissel/fpm#1833
🦋 Changeset detectedLatest commit: 2ea46e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 settings. |
Tested. Works well. |
Fixed a few more lint errors. All tests/lints should pass now. |
I'm pretty sure this is a breaking change. I rely on electron-builder/packages/app-builder-lib/src/appInfo.ts Lines 42 to 55 in 369f1fa
Why can't we retain current behavior? Why not just allow overriding with
|
@mmaietta yes, technically it would be a breaking change. I'm not sure there are any real-world scenarios where this might cause real-world issues though? I assumed this was a bug as
Isn't that pretty close to what we're doing in this PR? If That's my thinking on it but I'm happy to proceed with whatever direction you think makes sense. Unrelated to this PR but one final note about |
Hmmm That, by definition, is not a build number. I'm not sure how to solve for that edge case as it doesn't seem normal. We can add a way to override via metadata or config, but even that could be considered a breaking change |
@mmaietta how do you want me to proceed? I'm happy to make whatever changes you think are reasonable. The main thing that I want to do is fix #6945, everything else is just a personal preference. I guess the minimal changeset for that would be to just add If we want to do a non-breaking change then I guess we just add a |
This sounds perfect. Something like this below, but you may need to make other edits for the
Note, I'm fully in favor of this fix as it also helps remove some workarounds I made in my own project :) |
…userland#6945) explicit `buildNumber` will take precedence over any envs
@mmaietta Thanks! I've made the requested changes. Let me know if I'm missing anything. |
Looks good. Can you please add a changeset for this PR via |
@@ -129,9 +129,15 @@ export interface Configuration extends PlatformSpecificBuildOptions { | |||
*/ | |||
readonly npmRebuild?: boolean | |||
|
|||
/** | |||
* The build number. Maps to the `--iteration` flag for builds using FPM on Linux. | |||
* If not defined then will fallback to `BUILD_NUMBER` or `TRAVIS_BUILD_NUMBER` or `APPVEYOR_BUILD_NUMBER` or `CIRCLE_BUILD_NUM` or `BUILD_BUILDNUMBER` or `CI_PIPELINE_IID` env. |
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.
Minor grammar change (comma + 'it'): "If not defined, then it will fallback"
Thanks for also specifying its relationship to --iteration
/** | ||
* The build version. Maps to the `CFBundleVersion` on macOS, and `FileVersion` metadata property on Windows. Defaults to the `version`. | ||
* If `TRAVIS_BUILD_NUMBER` or `APPVEYOR_BUILD_NUMBER` or `CIRCLE_BUILD_NUM` or `BUILD_NUMBER` or `bamboo.buildNumber` or `CI_PIPELINE_IID` env defined, it will be used as a build version (`version.build_number`). | ||
* If `buildVersion` is not defined and `buildNumber` (or one of the `buildNumber envs) is defined, it will be used as a build version (`version.buildNumber`). |
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.
"`buildNumber envs" is missing a closing backtick around buildNumber
You'll need to re-run pnpm generate-all
after adjusting these jsdocs
"--iteration", | ||
// dashes are not supported for iteration in older versions of fpm | ||
// https://github.com/jordansissel/fpm/issues/1833 | ||
it.split("-").join("_") |
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.
Change to it.replaceAll('-', '_')
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.
replaceAll
isn't supported in Node 14.
@mmaietta Getting the following error while running Error log``` ➜ electron-builder git:(fix/fpm-iteration) ✗ pnpm generate-all
/Users/davejeffery/code/forks/electron-builder/packages/app-builder-lib/src/appInfo.ts (1,38): Cannot find module 'builder-util' or its corresponding type declarations. Error: No output definition. Probably caused by errors prior to this?
|
Hmmm, are you using a different version of typescript (installed globally)? I just ran a checkout of your PR via |
@mmaietta downgraded to node 14 (from node 16) and everything worked ok. See the latest commit based on your comments. |
Fixes #6945.
buildNumber
is not overridable in the config and is provided directly by the CI, this should bebuildVersion
instead.Dashes are not supported for iteration in some versions of fpm, so I replaced dashes with underscores. jordansissel/fpm#1833
Also, the linter complained, so I fixed up the type definition for the
use
function.