Skip to content

Conversation

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Aug 4, 2024

This runs lmms --version and lmms --help in the github CI. Further checks, like importing an LMMS project to WAV via CLI, are conceivable.

Note: This is a split of #6983 . Mutliple reviews have been done there, but one from DomClark is still outstanding, which I am reworking here.

Edit: CI is finally working, but I still need to rework the comments from review -> Keeping this a draft.

@PhysSong PhysSong requested a review from DomClark August 8, 2024 12:34
@JohannesLorenz JohannesLorenz marked this pull request as ready for review August 9, 2024 19:07
@JohannesLorenz JohannesLorenz marked this pull request as draft August 9, 2024 19:08
@JohannesLorenz
Copy link
Contributor Author

I tried everything. CI is just ignoring the build.yml at all 😢

@sakertooth
Copy link
Contributor

Looking at the workflow results, it says that there is an error at the bottom.

@JohannesLorenz
Copy link
Contributor Author

Ah, this can be seen only via the Actions tab. Thanks saker!

@JohannesLorenz
Copy link
Contributor Author

Status: I still need to care about proper downloading of the artifacts.

@JohannesLorenz JohannesLorenz force-pushed the fix-memleak-help branch 6 times, most recently from 7e8aa57 to 6d286ae Compare February 15, 2025 22:19
@JohannesLorenz
Copy link
Contributor Author

Will merge in 7 days if there are no complaints.

@tresf
Copy link
Member

tresf commented Feb 17, 2025

Will merge in 7 days if there are no complaints.

@JohannesLorenz I know you mentioned on Discord that you were awaiting some help from Dom in regards to testing lmms.exe on Windows... I'm happy to step in and do this for you however when I tested master on Windows and from my testing the terminal support is either broken or not obvious how to get something usable to STDOUT.

This leaves use a few options:

  1. Ignore and leave Windows alone for now
    --OR--
  2. Figure out why (Related; Allow console output on Windows if available #4719)
    --OR--
  3. Add a secondary lmms-console.exe target to our build system that enforces a compile with -mconsole, mentioned 8 years ago here.

@JohannesLorenz
Copy link
Contributor Author

Thanks for offering help.

however when I tested master on Windows and from my testing the terminal support is either broken or not obvious how to get something usable to STDOUT.

At least, I can say for sure that branch JohannesLorenz/ci-run-lmms, which was an old version where I did build and run in one script, worked fine on Windows, so the --help commandline output does work, probably just not on your machine (for whatever reasons).

If there are more complex solutions required to get it run for you, I would rather finish this for Linux and Mac. Like I said, I want to reduce the number of open PRs. A Windows solution can still be delivered later.

@tresf
Copy link
Member

tresf commented Feb 20, 2025

so the --help commandline output does work, probably just not on your machine (for whatever reasons).

This could be the case. I'm using Prism emulation on ARM. I'll see if it behaves differently on Wintel.

@tresf
Copy link
Member

tresf commented Feb 20, 2025

so the --help commandline output does work, probably just not on your machine (for whatever reasons).

This could be the case. I'm using Prism emulation on ARM. I'll see if it behaves differently on Wintel.

Nope, no luck. I can get some carla dll errors to show up, but that's about it. Maybe the log handler isn't being registered in time or something.

@JohannesLorenz JohannesLorenz marked this pull request as ready for review February 23, 2025 20:38
@tresf
Copy link
Member

tresf commented Feb 24, 2025

Maybe the log handler isn't being registered in time or something.

Using a local msys2 build, I tried moving the AttachConsole earlier in main.cpp, but I still can't get lmms.exe --help to work in PowerShell.

So I feel unless we want to either refactor main.cpp's --help to be a QCoreApplication or compile using -mconsole, the Windows portions of this task will have to wait until another time and should not hold up this PR.

Copy link
Member

@tresf tresf left a comment

Choose a reason for hiding this comment

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

Approve for macOS and Linux. Windows is sort of left in the dust, explained in more detail here: #7022 (comment)

This runs `lmms --help` in the github CI. Further
checks, like importing an LMMS project to WAV via CLI, are conceivable.
@tresf
Copy link
Member

tresf commented Mar 31, 2025

@JohannesLorenz since I haven't received any help over in #7732, I think we'll have to remove Windows from scope of this PR for now. If someone's able to help over on that PR to get a second binary for calling LMMS from console, we can re-add it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants