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

Always respect the shebangs of LilyPond's Python tools on macOS #1565

Merged
merged 4 commits into from
Mar 25, 2023

Conversation

jeanas
Copy link
Member

@jeanas jeanas commented Mar 24, 2023

According to the docstrings, a system Python was searched because the LilyPond-provided one did not function correctly. With current versions of LilyPond, which use a completely rewritten system for generating the binaries instead of GUB, it does.

Fixes #1492

According to the docstrings, a system Python was searched because the
LilyPond-provided one did not function correctly.  With current versions
of LilyPond, which use a completely rewritten system for generating the
binaries instead of GUB, it does.

Fixes frescobaldi#1492
Copy link
Collaborator

@dliessi dliessi left a comment

Choose a reason for hiding this comment

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

All these changes are fine, but there is one missing: we should delete the preference useshebang by means of install/update.py.
Anyway @jeanas you can already merge this PR: I'm going to add a follow-up commit (here if before you merge it, otherwise directly to master).

I am in favour of this change, but for the record I want to point out that this change may break the use of LilyPond's Python tools for older versions of LilyPond.
In my opinion this is not a problem, as convert-ly and imports usually make sense only with a recent version of LilyPond, if not the latest, and this PR has no effect on running LilyPond proper.

@jeanas
Copy link
Member Author

jeanas commented Mar 24, 2023

Good remark about install/update.py, I was starting to wonder about it. Given that it won't harm although it won't be used, can we just leave it there or should we delete it?

@dliessi
Copy link
Collaborator

dliessi commented Mar 24, 2023

You are right that unused preferences don't hurt, unless the same name is reused at a later time (the old preference value would be picked up). Even though that's only a theoretical risk, I would prefer to delete the unused preference entry.

@jeanas
Copy link
Member Author

jeanas commented Mar 25, 2023

OK, understood. I have added a commit that does this.

@fedelibre
Copy link
Member

Jean, I think you can merge it.

I don't see any other open PR ready to be included in tomorrow release.
Let's release 3.3.0 tomorrow, then we can make a bug fix release (3.3.1) in a few weeks to include other stuff.

I'll make the release tomorrow in the afternoon, Ok?
Then I would wait until Mac and Windows packages are ready (if it doesn't take too long) before announcing the release in the mailing lists.

@jeanas
Copy link
Member Author

jeanas commented Mar 25, 2023

Right now, I'm installing things in order to test this on @marnen's MacStadium node. If it looks OK, I will merge it.

The only remaining bug that I'd hope to fix before tomorrow is #1419 -- it's quite unfriendly for a newcomer to have an exception thrown if they try to compile a file before they understand that they must install LilyPond separately -- but it can also go into a bugfix release, no need to delay 3.3 longer.

@jeanas
Copy link
Member Author

jeanas commented Mar 25, 2023

#1567 is a simple fix for #1419.

@jeanas
Copy link
Member Author

jeanas commented Mar 25, 2023

The test worked fine. Let's merge this now.

@jeanas jeanas merged commit 6b8f436 into frescobaldi:master Mar 25, 2023
@jeanas jeanas deleted the force-shebang branch March 25, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants