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

Remove old AppVeyor and Travis CI files #877

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

DeinAlptraum
Copy link
Contributor

No description provided.

@DeinAlptraum
Copy link
Contributor Author

DeinAlptraum commented Jul 26, 2024

There is one more spot that could be changed perhaps (regarding Travis), src/base/TextGL.pas contains the following lines:

//if (Sections[FontNameIndex].StartsWith('Font_')) then // .StartsWith() does not compile on Travis-CI
if (LeftStr(FontSections[FontNameIndex], 5) = 'Font_') then
  begin
    SetLength(FontFamilyNames, Length(FontFamilyNames) + 1);
    //FontFamilyNames[FontNameIndex] := FontIni.ReadString(Sections[FontNameIndex], 'Name', Sections[FontNameIndex].Remove(0, 5)); // .Remove() does not compile on Travis-CI
    FontFamilyNames[FontNameIndex] := FontIni.ReadString(FontSections[FontNameIndex], 'Name', Copy(FontSections[FontNameIndex], 5));
  end;

Perhaps the "does not compile on Travis CI" comments should be either removed, or be reinstated

@barbeque-squared
Copy link
Member

That line was added commented from the go in #421 and there's more discussion about specifically this in #386. Especially that last one has multiple commits and comments mentioning stuff working essentially everywhere except Travis. My bets are on Travis using something super outdated.

I'm having a hard time figuring out in which particular fpc/sysutils/lazarus version that was added, but from what I read it's a compile-time error, so I've created https://github.com/UltraStar-Deluxe/USDX/tree/test-startswith to see what it does these days.

BE WARNED: it seems to build just fine, but I have no idea what it's actually supposed to do or if it works. There's more commented-out things near it, and at least LeftStr also shows up in UVideo, so there's a chance there's more hacks like this spread throughout the codebase.

If possible, it'd be great if the obvious ones (search for the commented-out functions) get fixed in this PR, that way the comment about Travis can get deleted for sure. But no need to go through the codebase looking for everything else that might have a more human-readable variant these days.

@DeinAlptraum
Copy link
Contributor Author

DeinAlptraum commented Jul 29, 2024

I changed the two mentioned occurrences, but

There's more commented-out things near it

Where exactly? I didn't see any similarly commented out code in the same file. Other than that, I looked for all occurrences of StartsWith, Remove and LeftStr but nothing is commented out, and I also did not find any other commented-out places when checking the changes in #421

I ran the game through one song quickly, which I assume is enough to check that this works (this was lyrics-font loading code)

On a sidenote, there's about 30 FIXMEs and 15 workarounds in the codebase

@barbeque-squared
Copy link
Member

Where exactly? I didn't see any similarly commented out code in the same file.

I meant that if you search for LeftStr in the entire repo, it should result in some non-commented line in UVideo that can probably be replaced with a StartsWith. Similarly, maybe there's other places that use Copy instead of Remove, which would be more human-readable (didn't check this though).

On a sidenote, there's about 30 FIXMEs and 15 workarounds in the codebase

That's probably not counting the dozens of TODO I've been adding (and if not: I have dozens for my local build for things that should really be fixed in USDX. It'll happen someday).

Needless to say, we're going to run into weird code, but with the comment gone, we'll tackle those as someone happens to be in that code area. I'll merge this later today.

@barbeque-squared barbeque-squared merged commit 72e8473 into UltraStar-Deluxe:master Jul 29, 2024
5 checks passed
@DeinAlptraum DeinAlptraum deleted the remove-old-ci branch July 29, 2024 19:03
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.

2 participants