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

CONFIGURE: Enable building with Windows text console output #3123

Merged
merged 1 commit into from Jul 13, 2021

Conversation

@trembyle
Copy link
Contributor

@trembyle trembyle commented Jun 30, 2021

Previously available on the wiki as a patch file for developers to apply to their configure script, this PR moves the option to a flag. The default behavior is for NO console output because users prefer that (for some weird reason).

--enable-windows-console and --disable-windows-console are the new flags. I'm open to suggestions if someone has a better name or description.

I noticed this when PR #3003 moved things around in configure, which caused the patch to stop working. If this PR is merged, I'll remove the patch from the wiki and suggest that devs use this flag instead.

I've tested this only on mingw-w32 with the console enabled, the console disabled by the new flag, and the console disabled as the default option.

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 1, 2021

Thanks for the patch! However, I think the text console should be enabled by default, since we got complaints from users about the missing console window that lead to introducing the custom SDL2 patch.

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Jul 1, 2021

I think release should not have it enabled

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 1, 2021

In this case, I think we need to reconsider how we do it in the Windows releases. Currently, we explicitly even create two Start Menu entries:

  • ScummVM
  • ScummVM (noconsole)

The 'default' entry opens the console window while the '(noconsole)' entry is required in case you don't want the console to show.

Since Windows 11 won't have the "old" Start Menu folders as we know them, it might be a good idea to change the behavior as @lephilousophe proposed: The debug builds should contain them (so we can easily get all required debug information) while most users won't need it in the release builds.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 1, 2021

I think it's a good setting to make explicit so that it's easier to use. Some notes:

  1. I think this conflicts with --enable-text-console. If that's enabled without a Windows console then instead of the GUI debugger appearing on hotkey or error, the non-existent console will wait on input and leave the SDL window bricked. We shouldn't allow that deadly combo. (I think we shipped a release like that once?)

  2. Why is this a find/replace on LIBS as opposed to append_var LDFLAGS "-mwindows"? Where is the existing -mconsole coming from? It's not explicitly set in configure. I don't know GCC environment variables well so this might be obvious to others, but I think it's worth a comment explaining why this has to be set in an unusual way. (Especially if the audience is ignorant Windows devs like me!)

  3. This shouldn't change the default, as @lotharsm said. Most mingw builds are by devs and devs want console output. If the goal is to change how releases and/or dev-releases are built, that's an entirely separate (and so far, intractable) conversation that's been going on for like a decade. It shouldn't derail this reasonable change.

ScummVM's subsystem PE header flag has bounced back and forth by subtle PRs like this one for years without consensus. Sometimes it would just get casually set in a formal release according to the preference of whoever prepared it. I don't see how that issue gets resolved here while the thread on PR #1405 lumbers on. [ and that's a pleasant thread! there's an old one that looks like it ended friendships! it's very sad. ]

We need consensus on whether or not Windows releases should be GUI apps or Console apps and whether that applies to dev releases too. Otherwise these threads will just keep happening. Either way is a trade-off so someone will always think it's wrong, and so far when that has become a bug report or a PR it gets accepted at face-value because we don't have something to point to that says why it is this way. The whole thing is also WinComplicated and the technical details are easy to get wrong, as I've done.

I may snap soon and write a wiki page laying what we have right now, how it works, what users should expect and what they shouldn't. At least then there would be something to point to. That Windows 11 detail, for example, is very interesting.

I care must less about which way we do it than I do about picking one and writing down why so that it doesn't keep flip flopping. The long history of this issue pains me! FWIW: I gave up long ago and my script that pulls down the daily win32 build also patches the PE header as I see fit, so hey at least I'm happy =)

@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 2, 2021

Just wanted to give a heads up that I'm in a car heading upstate for the US holiday, and I most likely won't be able to offer a considered response to this until next week.

@lotharsm and I briefly discussed on Discord. My provisional plan was to tie the console output to the --enable-release option, keeping the existing behavior for release builds while making the console default for developers.

@sluicebox you make a good point about --enable-text-console. I can check for that value as well so that they don't conflict.

@digitall
Copy link
Member

@digitall digitall commented Jul 4, 2021

@sluicebox : You don't need to write a wiki article on this issue discussing the pros and cons of this. We already have one of those:
https://wiki.scummvm.org/index.php/Windows/Console

@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 6, 2021

  1. Why is this a find/replace on LIBS as opposed to append_var LDFLAGS "-mwindows"? Where is the existing -mconsole coming from? It's not explicitly set in configure. I don't know GCC environment variables well so this might be obvious to others, but I think it's worth a comment explaining why this has to be set in an unusual way. (Especially if the audience is ignorant Windows devs like me!)

According to the GNU:

-mconsole
This option is available for Cygwin and MinGW targets. It specifies that a console application is to be generated, by instructing the linker to set the PE header subsystem type required for console applications. This is the default behaviour for Cygwin and MinGW targets.

So we shouldn't need to replace anything in LIBS. I'm changing this to append_var as you suggest.

EDIT: Would it be helpful to explicitly specify -mconsole as the default case?

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 6, 2021

@trembyle I'm not entirely sure and I need to verify, but IIRC it's SDL2 which pulls in -mwindows by default. That's the reason why the custom patch started with removing it straight from SDL2's .pc file.

@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 6, 2021

@trembyle I'm not entirely sure and I need to verify, but IIRC it's SDL2 which pulls in -mwindows by default. That's the reason why the custom patch started with removing it straight from SDL2's .pc file.

You're right. I just found that. I'll keep it as it was in the original patch.

@trembyle trembyle force-pushed the trembyle:master branch from abdce4b to 0d36e1c Jul 6, 2021
@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 7, 2021

@digitall omg thank you so much, that's hilarious that it already exists, thank you for sparing me work that i genuinely would have hated! and making it easy for me to link to on the next iteration!

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 7, 2021

@trembyle sorry i should have been clearer that i was just requesting a comment and not questioning the approach; i had immediately tried swapping out the find/replace for append_var and saw that it didn't work, so i knew what you were doing was necessary, just not why. thanks @lotharsm for the explanation of which component was injecting that.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 7, 2021

i don't think the _text_console test that you've added prevents the dire scenario I referenced that want to avoid; that change looks backwards. it's not --enable-windows-console that we're worried about; when there's a windows console we don't care if the scummvm debugging text console is enabled or not, both are valid and fine.

it's --disable-windows-console that's the problem; if that's the configuration and _text_console is yes then we're in trouble because now the scummvm debugging console is redirected to a text console that the user will never see.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 7, 2021

Is it now the intent of this PR to change the results of release builds so that there is no console? I am totally in favor of that (I have been for years!) but that will generate bug reports saying that it's a bug that running scummvm.exe from a command line doesn't produce text output and (incorrectly) claiming that this means that scummvm doesn't honor command line parameters. Bug reports like that are in part what caused us to switch from GUI releases to console releases, so if I seem like a crazy person desperately trying to prevent an infinite loop, it's because I am:

https://bugs.scummvm.org/ticket/5576
https://bugs.scummvm.org/ticket/6907
https://bugs.scummvm.org/ticket/7400
https://bugs.scummvm.org/ticket/9461
https://bugs.scummvm.org/ticket/10174
https://bugs.scummvm.org/ticket/11413
https://bugs.scummvm.org/ticket/10809
https://bugs.scummvm.org/ticket/11317

Just look at how many of those bugs were filed by people who've contributed code! Some of these have my own comments I'd forgotten about trying to explain this to no avail! And these are just the ones I could easily find while drinking!

We instruct people on the bug tracker homepage to run scummvm with command line parameters to print the version for bug reports. That's obviously fixable, but those are the kind of details we've flubbed in the past that have led to this. Where else do we have instructions that assume a responsive command prompt?

Much milder nitpick: Can we move the release-flag check above, or somehow adjust things, so that we don't echo ", Windows console" in cases where it doesn't get applied?

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 7, 2021

Is it now the intent of this PR to change the results of release builds so that there is no console? I am totally in favor of that (I have been for years!) but that will generate bug reports saying that it's a bug that running scummvm.exe from a command line doesn't produce text output and (incorrectly) claiming that this means that scummvm doesn't honor command line parameters. Bug reports like that are in part what caused us to switch from GUI releases to console releases, so if I seem like a crazy person desperately trying to prevent an infinite loop, it's because I am:

Thanks a lot for your comment!

Yes, the command line thingy is a quite... difficult discussion. I'm happy that this time it's really calm :-)

I think really disabling the console at all for the release builds is not an option due to the reasons you mentioned - no command line arguments, no easy way to print the version information. On the other hand, there's that command window sitting in the background each time you launch ScummVM - which might be annoying.

Currently, we ship the Windows releases with two Start Menu entries:

  • ScummVM
  • ScummVM (no console)

The default entry will start up ScummVM with the console window enabled, while the (no console) variant suppresses this by appending --noconsole to the startup command.

After consideration, I recommend the following solution for the next release:

  • We merge this PR. The default should be --enable-windows-console for both release and nightly.

For the presentation to the end user, I recommend the following change:

  • We use the --noconsole command for the default ScummVM Start Menu entry
  • Instead of the old (no console) entry, we add ScummVM (with console)

This is meant to slowly shift the users towards the hidden console window so that we can establish a new default here. Hopefully, we'll move more and more information that's relevant to the end user to the GUI so that they don't have to care about the console window anymore.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 9, 2021

@lotharsm I really like what you're proposing: changing the shortcuts so that "ScummVM" passes --no-console and swapping the "ScummVM (noconsole)" shortcut with "ScummVM (with console)". That's a great compromise that moves us in the right direction without giving up the console output / blocking that apparently so many Windows users crave in their release builds. (I don't!) The only cost is the flicker of a console window that quickly disappears, and as far as compromises go, that's not so bad for now.

I am also thankful that these are now the boring technical discussions they always should have been; sometimes you just need to wait things out =)

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Jul 9, 2021

What about having a --console flag which doesn't hide the console and hiding it by default? Maybe not hiding it when a command line argument is passed?
This lets users double clicking on the exe file to not have the console.
Using --help would let them see the --console flag in addition to the shortcut "(with console)"

What do you think?

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 9, 2021

I think that it's a bit too early now to change the default behavior when launching the exe - I bet most users still expect to see the console window in this case.

I propose to leave the default behavior unchanged until we gather some feedback after releasing the next stable version.

@trembyle trembyle force-pushed the trembyle:master branch from 0d36e1c to 54357c5 Jul 10, 2021
@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 10, 2021

i don't think the _text_console test that you've added prevents the dire scenario I referenced that want to avoid; that change looks backwards. it's not --enable-windows-console that we're worried about; when there's a windows console we don't care if the scummvm debugging text console is enabled or not, both are valid and fine.

@sluicebox I thought my last commit would fix this. Then I read your comment again and I'm doubting myself. Does my last change avoid this issue? If not, I will make amends.

Much milder nitpick: Can we move the release-flag check above, or somehow adjust things, so that we don't echo ", Windows console" in cases where it doesn't get applied?

I moved all of the checks to one place and added explanatory text for when text and Windows console conflict.

Also I fixed a completely unrelated minor typo that was bothering me! I'm hoping that's ok in this PR because it's such a small change.

@trembyle trembyle force-pushed the trembyle:master branch 2 times, most recently from 8708649 to 81c9c26 Jul 10, 2021
@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 10, 2021

This should now be working with the correct behavior and checks in place.

Let me know if those case statements are too ugly. They're only there so that I could use a wildcard for the host_os (which TIL you can't do with if conditions in shell scripts).

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 11, 2021

  1. This PR still changes behavior for release builds. At least @lotharsm and I are voting that that shouldn't happen here.
  2. Let's just ignore the conflict between _windows_console and _text_console for now. It's become a distraction on an important thread that I'd prefer keep focus. I think I'm in a better position to test that one so I'll take care of it after this is merged. Can you remove the stuff that tries to prevent the conflict? (and update the --enable-windows-console help message which is still backwards anyway)
@trembyle trembyle force-pushed the trembyle:master branch from 81c9c26 to fedc8b4 Jul 12, 2021
@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 12, 2021

  1. I agree with the consensus, but how does this PR change behavior for release builds?
  2. I updated the help message. I had tested many different scenarios (only in MinGW). Is there something else I can test that would make you feel comfortable that this is resolved? Would it help if I provided a detailed testing plan and results?
@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 12, 2021

@trembyle At this point, --enable-windows-console should always be the default, no matter if it is a release build or not. For the next release, we only change the presentation to the user managed by altering the shortcuts we install by default.

After that, we'll make the final decision if it should be enabled or disabled by default for the release builds based on user feedback.

This means that unless I explicitly use --disable-windows-console, the console should be enabled.

For me, this is the only remaining issue for now.

@trembyle trembyle force-pushed the trembyle:master branch from fedc8b4 to fce3d48 Jul 12, 2021
@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 12, 2021

@lotharsm Ok, I removed the check for release builds. To me this means that you're asking for a change in behavior, since the Windows console was not previously available as a build option.

As long as we agree on how it's supposed to work, it doesn't really matter if we call this a change or not.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 12, 2021

This looks good to me but I think the text (default for debug builds) and (default for release builds) should be removed from the usage text since that implies behavior that doesn't exist in the configure script. The configure script has no opinion or influence on the relationship between debug/release builds and consoles.

I'm really happy with the conversation this PR has sparked!

@trembyle trembyle force-pushed the trembyle:master branch from 8420ced to 7a30ce5 Jul 13, 2021
@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 13, 2021

This looks good to me but I think the text (default for debug builds) and (default for release builds) should be removed from the usage text since that implies behavior that doesn't exist in the configure script. The configure script has no opinion or influence on the relationship between debug/release builds and consoles.

🤦‍♂️I changed that description to fix the language around text console conflicts. Within minutes @lotharsm asked to change the release build behavior to match debug builds, and I forgot to change the help text again!

Anyway, fixed now!

@trembyle trembyle force-pushed the trembyle:master branch from 7a30ce5 to 6927fec Jul 13, 2021
Create a new flag --enable-windows-console to build with console
output on MinGW. The default behavior will be to enable the console.
The current plan is to continue to offer the users both options for
release builds - ScummVM and ScummVM (noconsole).

Whenever the text console is enabled, Windows console will be added
by default. This is to prevent a situation where ScummVM will not load
any visible debug console on hotkey or error.
@trembyle trembyle force-pushed the trembyle:master branch from 6927fec to 862c8bb Jul 13, 2021
@trembyle
Copy link
Contributor Author

@trembyle trembyle commented Jul 13, 2021

I squashed everything together to clean up the commit history. Any remaining issues?

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Jul 13, 2021

This looks great, prevents the problematic combo of a text-console on a GUI build, doesn't change any defaults, and we got a productive discussion out of it.

With the commits squashed, this can be merged now. Thanks for your work on this! =)

@sluicebox sluicebox merged commit 63b8d47 into scummvm:master Jul 13, 2021
8 checks passed
8 checks passed
@github-actions
Windows (win32, x86-windows, x86, --enable-faad --enable-mpeg2 --enable-discord --disable-fribidi...
Details
@github-actions
Windows (x64, x64, x64-windows, --enable-faad --enable-mpeg2 --enable-discord --disable-fribidi, ...
Details
@github-actions
Windows (arm64, arm64, arm64-windows, --enable-faad --enable-mpeg2 --enable-discord --disable-fri...
Details
@github-actions
Xcode (macosx, -scheme ScummVM-macOS, --disable-nasm --enable-faad --enable-mpeg2, a52dec faad2 f...
Details
@github-actions
Xcode (ios7, -scheme ScummVM-iOS CODE_SIGN_IDENTITY="" CODE_SIGNING_ALLOWED=NO, --disable-nasm --...
Details
@github-actions
Ubuntu (ubuntu-latest, sdl2-config, --enable-c++11, libsdl2-dev libsdl2-net-dev liba52-dev libjpe...
Details
@github-actions
Ubuntu (ubuntu-18.04, sdl-config, --disable-c++11, libsdl1.2-dev libsdl-net1.2-dev liba52-dev lib...
Details
@codacy-production
Codacy Static Code Analysis Codacy Static Code Analysis
Details
@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 14, 2021

Indeed, this really looks great and helps moving the port forward!

Thank you very much for keeping this discussion so calm reasonable 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants