Skip to content
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

Add branding features on MacOS PKGs #469

Merged
merged 30 commits into from Jul 27, 2022
Merged

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Oct 14, 2021

Branding

MacOS PKGs were still using the default Anaconda branding and texts, so they couldn't be customized. I took some logic used in the Windows installers to provide equivalent features in this target output. I also needed to add some new keys.

Note that if these new keys are not provided, Anaconda's default branding is still used; that way no backwards compatibility is broken for anybody relying on that (if this is being used internally or something). To disable Anaconda's branding you need to be explicit about it with empty strings "" (null fields are pruned for some reason during parsing).

New keys:

  • welcome_file, welcome_text: Shown at the beginning of the wizard.
  • readme_file, readme_text: Shown right before the license if provided.
  • conclusion_file, conclusion_text: Shown at the end of the wizard.

Reused keys (from Windows)

We have also added:

  • reverse_domain_identifier, so it doesn't hardcode io.continuum internally.
  • default_location_pkg and pkg_name so you can better customize the default installation path. This is not as flexible as Windows or Linux options, but it's the best we can do for now. Closes default_prefix is not working for pkg #432 (in a way)
  • install_path_exists_error_text, which allows you to change the default error message shown when the install path already exists. This way you can tell your users to use a specific mechanism for your app, if needed.
  • notarization_identity_name. We sign the bundled conda-standalone binary so it can pass notarization too. This requires a different certificate, so this key was added.

Other cosmetic fixes

  • The installer will now report the total installation size accurately. Before, it misreported the compressed tarballs size.
  • The progress bar fills up way too fast up to 90% (little we can do here), where it spends most of the time (conda is running behind the scenes). We use osascript to send notifications and report progress so the users don't think it got stuck.

Other technical fixes

  • Install the PKG as part of the examples CI. This only happens on actual CI pipelines, not locally, because the PKG mechanism doesn't allow for arbitrary target locations, so we would end up polluting the user home directory. On local runs, the previous pkgutil expand behaviour is kept (unless the user chooses to run the PKG installation, which can be enabled by setting CI=1 to workaround the CI checks).

@anaconda-issue-bot anaconda-issue-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 14, 2021
@jaimergp jaimergp mentioned this pull request Nov 22, 2021
@hoechenberger
Copy link
Contributor

Hello all, I just wanted to ask whether this PR might be considered for inclusion? This is a feature I'd love to see in constructor, and @jaimergp has put in a lot of effort to get it done. Is there still something missing? Please let me know if and how I can be of any help to get this PR merged.

Thank you!

Richard

@hoechenberger
Copy link
Contributor

Super friendly bump :) I can offer to test things on my macOS machine!

@jaimergp
Copy link
Contributor Author

@hoechenberger Let's sync on the tests! Reach out on gitter.im; I have some builds you can try.

@hoechenberger
Copy link
Contributor

@jaimergp I'm busy today but will reach out tomorrow (Friday UTC)!

@hoechenberger
Copy link
Contributor

I've tested the new features added by this PR, and it's all working fine for me with the recent fix @jaimergp added to menuinst.

I believe this is good to go, but please ping me if you need further testing!

@hoechenberger
Copy link
Contributor

@jaimergp LMK if / when I should test this again!

@jaimergp
Copy link
Contributor Author

I am preparing another PR to have the installers uploaded as artifacts so we can test/debug locally in an easy way. See #498!

@jaimergp jaimergp closed this Mar 14, 2022
@hoechenberger
Copy link
Contributor

Nice! Any chance to see this merged into main anytime soon? 👍

Copy link

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable and as I mentioned in #497 we've been using this successfully for a while, so +1 for merge from me. Let's leave it open for another day or two to give people a chance to look and/or ask for more time, then click the green button if nobody else has comments!

Copy link
Contributor

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

LGTM!!

@larsoner
Copy link

larsoner commented Jul 7, 2022

@jaimergp WDYT about my testing ideas in #525 (comment) ? I wonder if we should try to implement something like this in the next few days first, then rebase this PR to use the new testing framework.

We could even test the notarization stuff using private secrets if you or @hoechenberger are willing to put your dev credentials up here. We could use them only on main builds, not on PRs, to keep them safe I think. That way we'd at least see failures with the signing on main builds...

Copy link
Contributor

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @jaimergp !

🚀

@jaimergp
Copy link
Contributor Author

@larsoner Yes! Good idea, improving the tests on constructor first sounds very reasonable. I've tried to add items to examples so at least we hit some code paths during creation and installation, but we don't currently have a way to ensure the installation ended up behaving in the way we wanted. Maybe with some post_install.sh scripts, but that's very clunky and limited. I like the fixture approach better.

@larsoner
Copy link

've tried to add items to examples so at least we hit some code paths during creation and installation, but we don't currently have a way to ensure the installation ended up behaving in the way we wanted

Are all the examples already run by the CIs? If so, I think that's a good enough start to merge this and #507.

If they aren't run by CIs, maybe a quick PR to make some CI run run the examples on the appropriate OSes would be a good start. Would you be up for making one?

@jaimergp
Copy link
Contributor Author

Yes, they are being run. Check this one, for example.

The only problem I see is that:

  • A) There's no exhaustive test of every possible combination of options.
  • B) Some of the features here implemented require visual confirmation.

However, since we've been using both for a while, I think we are okay.

I would love to see someone from Anaconda drop a review though, if possible. I know SciPy is happening right now, but maybe they can pass by in one of the sprints. What do you think @jezdez?

@larsoner
Copy link

larsoner commented Jul 14, 2022

Yes, they are being run.

Great!

A) There's no exhaustive test of every possible combination of options.

I think this will be true of any sufficiently complex system (with sufficiently time-consuming processing) so I think it's okay to continue trying to cover at least the most common use cases/combos

B) Some of the features here implemented require visual confirmation.

This will be tough to unit test :(

I would love to see someone from Anaconda drop a review though, if possible. I know SciPy is happening right now, but maybe they can pass by in one of the sprints. What do you think @jezdez?

Also @jezdez let me know if you or anyone else at the Anaconda end would prefer to look at all PRs before some non-Anaconda person (like me!) merges them let me know!

My plan at least was to look at and merge changes that we have been using already at the MNE-Python end successfully for 6-12 months already (having adopted them from the napari folks) and maybe others if I get familiar enough with the codebase.

@scottwsides
Copy link

I hate to pile on... but whats needed to merge these changes?

@jaimergp
Copy link
Contributor Author

I'll merge next Monday (ping me if I forget!). We can always revisit changes before a release if needed ;) This PR introduces quite a few configuration keys so I was maybe expecting a bit of pushback but nobody said anything so... 🤷

scripts/run_examples.py Outdated Show resolved Hide resolved
@scottwsides
Copy link

Hey... your changes work and they're awesome. hard to push back on that. ;-)

@jaimergp jaimergp merged commit f0b0f88 into conda:main Jul 27, 2022
@scottwsides
Copy link

Hey this is great... thanks! I'll test this now on mac. Just to push my luck it'd be great if all these changes were docmented on the main github page .

@jaimergp
Copy link
Contributor Author

Yes, I think the docs at CONSTRUCT.md need to be re-rendered. There are more comments in the examples/osxpkg example ;)

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Jul 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

default_prefix is not working for pkg Feature request: make logo in mac installer customizable
7 participants