Skip to content

Conversation

phoerious
Copy link
Member

@phoerious phoerious commented Dec 19, 2023

Work-in-progress PR for rewriting out release-tool in Python and merging the Bash and PS1 versions.

Type of change

  • ✅ New feature (change that adds functionality)

@phoerious phoerious added this to the v2.8.0 milestone Dec 19, 2023
@phoerious phoerious force-pushed the feature/python-release-tool branch 2 times, most recently from b460067 to c423ae6 Compare December 19, 2023 22:59
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.35%. Comparing base (6f59444) to head (419de58).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #10114   +/-   ##
========================================
  Coverage    64.35%   64.35%           
========================================
  Files          378      378           
  Lines        39719    39719           
========================================
  Hits         25561    25561           
  Misses       14158    14158           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@phoerious phoerious force-pushed the feature/python-release-tool branch 25 times, most recently from a56d67e to 0732119 Compare March 10, 2025 16:34
@phoerious phoerious force-pushed the feature/python-release-tool branch 11 times, most recently from f52c79c to d741087 Compare March 11, 2025 17:36
@droidmonkey droidmonkey force-pushed the feature/python-release-tool branch from 7fd7a73 to 85038ad Compare March 30, 2025 12:21
phoerious and others added 9 commits October 13, 2025 19:35
Fix argparse handling when no subcommand given

Implement check command

Implement i18n command

Add Ctrl+F TOC

Implement merge command

Implement source tarball creation

Implement macOS builds

Implement Linux builds

Support building for multiple platforms

Disable test building

Debug-log executed commands

Allow cross-compilation without Docker when using vcpkg

Implement macOS codesigning and notarization

Remove obsolete globals

Check xcode setup

Implement GPG signing

Re-enable Git branch checkout

Show key selection for merge commit signing

Check for git and merge basic and tool checks

Remove redundant checkout message

Update headline formatting
* Move pre-build installer signing to cmake to streamline the operation and avoid signing hundreds of unnecessary files.
* Enable building both amd64 and arm64 builds with visual studio
* Incorporate shell=True directly into _run when on windows and modifying path/env
* Fix _get_bin_path to resolve the build_dir to absolute path
* Fix _git_branches_related to ignore non-zero return code
* Fix double naming of transifex resource
@droidmonkey droidmonkey force-pushed the feature/python-release-tool branch from e728c4d to 419de58 Compare October 13, 2025 23:35
@droidmonkey droidmonkey marked this pull request as ready for review October 13, 2025 23:36
@Copilot Copilot AI review requested due to automatic review settings October 13, 2025 23:36
@droidmonkey
Copy link
Member

droidmonkey commented Oct 13, 2025

@phoerious this is ready for prime time. I tested all functions except for merge on windows. Made several improvements and fixes. Will test merge soon.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a comprehensive Python rewrite of the KeePassXC release tool, replacing the existing Bash and PowerShell versions with a unified Python implementation. The new tool consolidates functionality for checking, merging, building, signing, and internationalization tasks while adding improved cross-platform support.

  • Python-based unified release tool replacing separate Bash and PowerShell scripts
  • Enhanced Windows build system with integrated code signing capabilities
  • Updated Transifex configuration for modern API endpoints

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
release-tool.py New comprehensive Python release tool with all functionality from previous scripts
release-tool.ps1 Removed PowerShell version (replaced by Python tool)
release-tool Removed Bash version (replaced by Python tool)
share/linux/appimage-apprun.sh New AppImage launcher script for Linux distributions
src/CMakeLists.txt Updated Windows packaging to use new post-install script
cmake/WindowsPostInstall.cmake.in New Windows post-install script template for signing and portable zip creation
cmake/MakePortableZip.cmake Removed old portable zip creation script
.tx/config Updated Transifex configuration for new API endpoints

raise Error('No secret keys found!')

if not user_choice and len(keys) == 1:
logger.info('Using secret key %s %s.', keys[0][0]. keys[0][2])
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

Missing comma operator between tuple elements. Should be keys[0][0], keys[0][2] instead of keys[0][0]. keys[0][2].

Suggested change
logger.info('Using secret key %s %s.', keys[0][0]. keys[0][2])
logger.info('Using secret key %s %s.', keys[0][0], keys[0][2])

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

wtf

changelog = 'Release ' + changelog.group(1)

tag_name = tag_name or version
logger.info('Creating "%s%" tag...', tag_name)
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

Incorrect string formatting with mismatched quotes. Should be 'Creating \"%s\" tag...' or use a different quote style to avoid the %s% pattern.

Suggested change
logger.info('Creating "%s%" tag...', tag_name)
logger.info('Creating "%s" tag...', tag_name)

Copilot uses AI. Check for mistakes.

yes_no = input().strip().lower()
if default_no:
return yes_no == 'y'
return yes_no == 'n'
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

Logic error in _yes_no_prompt function. When default_no=False (meaning default is 'yes'), the function should return yes_no != 'n' to return True unless user explicitly says 'n'.

Suggested change
return yes_no == 'n'
return yes_no != 'n'

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, this is a logic error

signtool_args += ['/sha1', identity]
signtool_args += ['/d', file.name, str(file.resolve())]

_run(signtool_args, cwd=src_dir, capture_output=False, shell=True)
Copy link

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

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

Using shell=True with user-controlled input (identity parameter) poses a command injection risk. The signtool_args list should be executed directly without shell=True.

Suggested change
_run(signtool_args, cwd=src_dir, capture_output=False, shell=True)
_run(signtool_args, cwd=src_dir, capture_output=False)

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

This is run locally, no risk for command injection unless you do it to yourself 😅

Copy link
Member Author

@phoerious phoerious Oct 14, 2025

Choose a reason for hiding this comment

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

shell should still be set to false. I don't see why this needs to be run in a shell wrapper.

Copy link
Member

@droidmonkey droidmonkey Oct 14, 2025

Choose a reason for hiding this comment

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

Because it literally didn't work otherwise. The sign tool will invoke a pin prompt and that will fail to show with an error if not done in a shell

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, perhaps add a comment then.

Comment on lines +273 to +275
inst = subprocess.check_output([vswhere, '-latest', '-products', '*',
'-requires', 'Microsoft.Component.MSBuild',
'-property', 'installationPath'], text=True).strip()
Copy link
Member Author

Choose a reason for hiding this comment

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

Why not use _run(...).stdout here?

Copy link
Member

Choose a reason for hiding this comment

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

I wrote this before I understood all the functions in this file, good point

Comment on lines +301 to +303
out = subprocess.run(f'cmd /c "{vs_cmd}" -arch={arch} -no_logo && set', capture_output=True, text=True)
except subprocess.CalledProcessError as e:
raise Error('Failed to run Visual Studio dev script: %s', e.output or str(e))
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here. I would also separate the cmd call and the actual shell command like this:

_run(['cmd', '/c', f'"{vs_cmd}" -arch={arch} -no_logo && set']), text=True)

Also, maybe escaping spaces vs_cmd.replace(' ', r'\ ') is safer than quotes?

You should probably also call _find_vs_dev_cmd from here directly. There's only a single usage of both these functions, so no need to take vs_cmd as a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I tried so many ways to get all this to work.... out of 100 possible permutations only 2 work because windows command prompt is trash

Comment on lines +306 to +309
for line in out.stdout.splitlines():
k, v = line.split('=', 1)
if v is not None:
env[k] = v
Copy link
Member Author

Choose a reason for hiding this comment

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

v will never be None. This will just error out if there's no =.

Maybe a better solution than Copilot's would be not to use expansion at all:

Suggested change
for line in out.stdout.splitlines():
k, v = line.split('=', 1)
if v is not None:
env[k] = v
for line in out.stdout.splitlines():
if len(kv := line.split('=', 1)) == 2:
env[kv[0]] = kv[1]

Copy link
Member

Choose a reason for hiding this comment

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

In my testing it didn't error out, only k had a value

Copy link
Member Author

@phoerious phoerious Oct 14, 2025

Choose a reason for hiding this comment

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

For 'abc=' yes (but then it'll be '', not None). For 'abc' you will get a ValueError.

Comment on lines +686 to +687
vs_cmd = _find_vs_dev_cmd()
vs_env = _capture_vs_env(vs_cmd, arch=platform_target)
Copy link
Member Author

Choose a reason for hiding this comment

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

Merge (see above)

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.

2 participants