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

Rename builds to reduce confusion #74699

Closed
wants to merge 2 commits into from

Conversation

RenechCDDA
Copy link
Member

@RenechCDDA RenechCDDA commented Jun 21, 2024

Summary

Infrastructure "Builds renamed for clarity"

Purpose of change

In the development discord's #player-help channel alone there are 202 mentions (282 including bot posting download links) of the phrase msvc. This is not something that should ever be a concern for the average player, but it often is. People find it very confusing, as this is information only truly relevant to development and well... most people interacting with CDDA simply don't have to deal with it.
image

Describe the solution

Rename the builds. tiles was replaced with graphics, a with/no was added before sound to clarify, and msvc was replaced with alternate.

Also drop the x64 specifier, since 32-bit support has been dropped for over a year now. It is retained in the android build names, as apparently we still build x32 there (?)

cdda-windows-tiles-sounds-x64 --> cdda-windows-graphics-with-sound
cdda-windows-tiles-sounds-x64-msvc --> cdda-windows-graphics-with-sound-alternate

etc.

This is still not very helpful to the average person but hopefully they will be able to realize that alternate implies interchangeable (for them), and reduce the amount of friction in the process of downloading the game.

Describe alternatives you've considered

Akrieger wants to kill the non-msvc builds altogether. That would help.

Testing

I have no ability to test that this doesn't blow up the builds outside of my own local environment, so this should absolutely wait for all CI to pass successfully. Failures reported in the build process very likely indicate something has gone wrong.

Additional context

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. Code: Build Issues regarding different builds and build environments [JSON] Changes (can be) made in JSON [Markdown] Markdown issues and PRs Code: Tooling Tooling that is not part of the main game but is part of the repo. Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jun 21, 2024
@Zireael07
Copy link
Contributor

This should only happen once you do "kill the non-msvc build".

There were cases of bugs only happening on msvc or only on non-msvc so as long as there are two compilers the info is necessary if a bug crops up

@PatrikLundell
Copy link
Contributor

"Alternative" to me indicates it's a fallback to use if the "real" version doesn't work for whatever reason. "msvc" at least provides some clue as to what the difference is (if you can guess what it means, when the average play-only person probably can't). I would probably rather have a suffix on the "normal" version to indicate what it is (gcc?).

@RenechCDDA
Copy link
Member Author

This should only happen once you do "kill the non-msvc build".

There were cases of bugs only happening on msvc or only on non-msvc so as long as there are two compilers the info is necessary if a bug crops up

It's not like they're going to have the same name? You'd still tell them "use the alternate version". That happens very rarely regardless, and many more people (100% of downloaders, generally!) will be encountering the build names. So I would think it's prudent to simplify the part that everyone is guaranteed to encounter.

@Qrox
Copy link
Contributor

Qrox commented Jun 21, 2024

If we do not mind longer names, it might be clearer to use -built-with-msvc/gcc as the suffix, so that the player will hopefully conclude that msvc/gcc is some sort of tool used to build the game, rather than something inherent to the game.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jun 21, 2024
@RenechCDDA
Copy link
Member Author

If we do not mind longer names, it might be clearer to use -built-with-msvc/gcc as the suffix, so that the player will hopefully conclude that msvc/gcc is some sort of tool used to build the game, rather than something inherent to the game.

Unfortunately I believe that still lends itself to the layman's question of "What is the difference?" What it is specifically built with is wholly irrelevant to them, introducing that information only adds confusion.

@Qrox
Copy link
Contributor

Qrox commented Jun 22, 2024

the layman's question of "What is the difference?"

I think the only way to avoid that is to provide only one version of the windows build. Otherwise people might still be curious and ask questions, so letting them know from the name that the difference is the build tool can at least satisfy some of them.

@andrei8l
Copy link
Contributor

This largely feels like change for its own sake.

In the development discord's #player-help channel alone there are 202 mentions (282 including bot posting download links) of the phrase msvc

Why is this a problem?

tiles was replaced with graphics

The build flag is TILES in all 3 build systems, so the resulting package should say tiles, if anything.

a with/no was added before sound to clarify,

I'm still not sure why we have a no-sounds build. Sound is supported on all platforms that support tiles. Tiles should imply sound.

Akrieger wants to kill the non-msvc builds altogether. That would help.

This is the correct solution.

@Zireael07
Copy link
Contributor

@RenechCDDA I already explained why the difference. It is not irrelevant if a bug crops up.

@RenechCDDA
Copy link
Member Author

RenechCDDA commented Jun 22, 2024

Why is this a problem?

Because it is a constant source of confusion fielding many questions and much unnecessary time. That is not counting the invisible cost of players which turn away at the download screen because they can't choose.

The build flag is TILES in all 3 build systems, so the resulting package should say tiles, if anything.

I don't see any reason why the build flag should be prescriptive. The documentation seems to be clear on the difference, and I would expect that new contributors building with custom build flags are either capable of reading the documentation or figuring it out on their own. I would not expect our random players to - which is why we have hundreds of instances of players being confused.

If it turns out to be confusing to devs I can make another PR to switch the build flags, that just seems to be unnecessary as I've never seen someone compiling that is confused between curses/tiles.

I'm still not sure why we have a no-sounds build.

This is the correct solution.

I don't think we have anyone raising the banner for that right now, so this is a cheap easy win. I am happy to close my PR if a better alternative appears. But until some effort is made to clearly differentiate them or remove the difference, the confusion will continue.

@RenechCDDA I already explained why the difference. It is not irrelevant if a bug crops up.

I do not see any practical difference between these messages.

"Try using the MSVC build instead"

"Try using the alternate build instead. [If you are already using the alternate build, try using the normal build. If you don't know which one you have, check the downloaded file's names or better yet try both.]"

If saying "alternate build" is confusing you can take the time to explain... in that very rare circumstance. That is better than having to explain the difference between MSVC build and not-MSVC build hundreds of times.

@Qrox
Copy link
Contributor

Qrox commented Jun 23, 2024

I'm still not sure why we have a no-sounds build. Sound is supported on all platforms that support tiles. Tiles should imply sound.

I think the build without sound still supports sound, it just come without a sound pack to reduce size.

@andrei8l
Copy link
Contributor

andrei8l commented Jul 1, 2024

Because it is a constant source of confusion fielding many questions and much unnecessary time.

I don't think 200 mentions of msvc indicates confusion or wasted time. From a quick look, a significant portion of those aren't even questions, but rather statements ("use msvc one" or "don't use msvc one"). Kevin is mentioned 380 times and since we're not looking at context, let's remove Kevin too because they're confusing.

That is not counting the invisible cost of players which turn away at the download screen because they can't choose.

I don't think our users need to be coddled quite so hard. Whichever build they download, they'll get a working game with only minor technical differences, most of the time. You can also address this with a manual note for releases, like this:

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 3d00a0b20c..2b132854f5 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -45,7 +45,9 @@ jobs:
         id: generate-release-notes
         run: |
           npm install @actions/github
-          node build-scripts/generate-release-notes.js '${{ secrets.GITHUB_TOKEN }}' '${{ steps.generate_env_vars.outputs.tag_name }}' "$(git log -1 --format='%H')" > notes.txt
+          echo "# A note for Windows users:" > notes.txt
+          echo "The msvc build is the recommended one. Don't use the mingw build unless you're tracking down bugs" >> notes.txt
+          node build-scripts/generate-release-notes.js '${{ secrets.GITHUB_TOKEN }}' '${{ steps.generate_env_vars.outputs.tag_name }}' "$(git log -1 --format='%H')" >> notes.txt
       - run: |
           gh release create ${{ steps.generate_env_vars.outputs.tag_name }} --notes-file notes.txt --prerelease --title "${{ steps.generate_env_vars.outputs.release_name }}" --target "$(git log -1 --format='%H')"
 
and it looks like this

Screenshot from 2024-07-01 18-06-25

@RenechCDDA
Copy link
Member Author

I think I've failed to convey why I opened this pull request. I mentioned the mentions of msvc in the development discord, but that is not why I opened this PR. I intended to put that forward as supporting evidence, i.e. "Look at how often this is, it's not just my opinion that it happens a lot"

I, personally, have seen many cases of new players confused by this. And for every case I do see, I am sure there are more that I do not, including many of the ones that appear in the development discord.

@zakhad
Copy link

zakhad commented Nov 15, 2024

Does the catapult launcher need to be updated as well as i don't get builds past this
image

@RenechCDDA
Copy link
Member Author

@zakhad This PR wasn't even merged. I don't know why you are commenting here.

@zakhad
Copy link

zakhad commented Nov 15, 2024

@RenechCDDA Sorry i had multiple issues up it seems that i posted in the wrong one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Code: Build Issues regarding different builds and build environments Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tooling Tooling that is not part of the main game but is part of the repo. <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants