-
Notifications
You must be signed in to change notification settings - Fork 176
Briefcase installer options #1144
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
base: briefcase-integration
Are you sure you want to change the base?
Briefcase installer options #1144
Conversation
7ae9015 to
532ff27
Compare
constructor/briefcase.py
Outdated
| ) | ||
| initialize_conda = info.get("initialize_conda", "classic") | ||
| if initialize_conda: | ||
| # TODO: How would we distinguish between True/classic in the UI? Same for NSIS |
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.
True and classic are synonyms. You would have to distinguish between classic and condabin. You can currently just take what's in the NSIS template and add that here.
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.
Fixed 326be7f
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.
I was thinking about these lines:
constructor/constructor/nsis/OptionsDialog.nsh
Lines 167 to 178 in 97dbba0
| # Account for the conda mode | |
| ${If} "${INIT_CONDA_MODE}" == "condabin" | |
| ${NSD_CreateLabel} 5% "$5u" 90% 20u \ | |
| "Adds condabin/, which only contains the 'conda' executables, to PATH. \ | |
| Does not require special shortcuts but activation needs \ | |
| to be performed manually." | |
| ${Else} | |
| ${NSD_CreateLabel} 5% "$5u" 90% 20u \ | |
| "NOT recommended. This can lead to conflicts with other applications. \ | |
| Instead, use the Commmand Prompt and Powershell menus added \ | |
| to the Windows Start Menu." | |
| ${EndIf} |
You can then add the condabin UI text if info.get("initialize_conda", "classic") == "condabin", and the other text if not.
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.
Ah, updated a642278
constructor/briefcase.py
Outdated
| { | ||
| "name": "register_python", | ||
| "title": "Register Python as System Default", | ||
| "description": "TODO: Register Python description", |
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.
For now, you can just copy the description in NSIS.
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.
Fixed 326be7f
26ee3bb to
a2caeec
Compare
a642278 to
575b454
Compare
|
@marcoesters I've now rebased this branch, even though this does not connect the UI installer options right now to any sort of "activity" in |
marcoesters
left a comment
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.
It looks like a few options are missing:
constructor/constructor/nsis/OptionsDialog.nsh
Lines 9 to 13 in 5eecfa0
| Var mui_AnaCustomOptions.AddToPath | |
| Var mui_AnaCustomOptions.PostInstall | |
| Var mui_AnaCustomOptions.PreInstall | |
| Var mui_AnaCustomOptions.ClearPkgCache | |
| Var mui_AnaCustomOptions.CreateShortcuts |
constructor/briefcase.py
Outdated
| options = [] | ||
| register_python = info.get("register_python", True) | ||
| if register_python: | ||
| python_version = f"{sys.version_info.major}.{sys.version_info.minor}" |
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.
This will be the Python version of the Python binary used to call constructor. This is not necessarily the same as the one bundled in the installer - in fact, python may not even be in the installer at all. You will have to use _dists in the info dictionary to get the Python version bundled in the installer.
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.
While looking into info["dists"], I found this seemingly old code in winexe.py:
make_nsi(
{
"name": "Maxi",
"version": "1.2",
"_platform": "win-64",
"_outpath": "dummy.exe",
"_download_dir": "dummy",
"_dists": ["python-2.7.9-0.tar.bz2", "vs2008_runtime-1.0-1.tar.bz2"],
},
".",
)
Is it safe to assume that if an installer bundles python, the package is formatted as above? I.e. python-x.y.z-<build number>.tar.bz2?
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.
In general, package names are formatted as <package name>-<version>-<build string>.<extension>. <build string> is not necessarily a number and <package name> may contain dashes. <extension> is either .tar.bz2 or .conda, but that may change in the future.
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.
See 6087f5e
|
@marcoesters I've added the remaining options in 6087f5e, I compared it to |
marcoesters
left a comment
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.
Just a few small comments
| if has_python: | ||
| register_python = info.get("register_python", True) | ||
| if register_python: | ||
| options.append( | ||
| { | ||
| "name": "register_python", | ||
| "title": f"Register {info['name']} as my default Python {python_version}.", | ||
| "description": "Allows other programs, such as VSCode, PyCharm, etc. to automatically " | ||
| f"detect {info['name']} as the primary Python {python_version} on the system.", | ||
| "default": info.get("register_python_default", False), | ||
| } | ||
| ) |
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.
| if has_python: | |
| register_python = info.get("register_python", True) | |
| if register_python: | |
| options.append( | |
| { | |
| "name": "register_python", | |
| "title": f"Register {info['name']} as my default Python {python_version}.", | |
| "description": "Allows other programs, such as VSCode, PyCharm, etc. to automatically " | |
| f"detect {info['name']} as the primary Python {python_version} on the system.", | |
| "default": info.get("register_python_default", False), | |
| } | |
| ) | |
| if has_python and info.get("register_python", True) | |
| options.append( | |
| { | |
| "name": "register_python", | |
| "title": f"Register {info['name']} as my default Python {python_version}.", | |
| "description": "Allows other programs, such as VSCode, PyCharm, etc. to automatically " | |
| f"detect {info['name']} as the primary Python {python_version} on the system.", | |
| "default": info.get("register_python_default", False), | |
| } | |
| ) |
| script_description = info.get(f"{script_type}_install_desc", "") | ||
| script = info.get(f"{script_type}_install", "") | ||
| if script_description and not script: | ||
| raise ValueError( |
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.
| raise ValueError( | |
| raise KeyError( |
Technically since it is a key in construct.yaml?
| raise FileNotFoundError( | ||
| f"Specified {script_type}-install script '{script}' does not exist." | ||
| ) | ||
| if not is_bat_file(script_path): |
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.
I think you can just check for the suffix here. The only thing is_bat_file does additionally is checking whether the file exists - which you do immediately before this.
Description
This does not fully resolve #1103 but is a start.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?