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

Fix invalid filename #11511

Merged
merged 2 commits into from
Feb 27, 2023
Merged

Fix invalid filename #11511

merged 2 commits into from
Feb 27, 2023

Conversation

RoiEXLab
Copy link
Member

This should hopefully address the problem with #11413

/cc @DanVanAtta

@RoiEXLab
Copy link
Member Author

@DanVanAtta It seems like the invalid character in the file caused it to not be applied at all. Fixing it cause the build to fail, so I just removed the file. Now you should be able to check out the repository on windows again.

@RoiEXLab RoiEXLab merged commit ced49fa into master Feb 27, 2023
@RoiEXLab RoiEXLab deleted the fix-invalid-filename branch February 27, 2023 23:32
@DanVanAtta
Copy link
Member

Any idea what the invalid character was?
Can we fix this without nuking the file?
Perhaps there is something we can add in .gitattributes?

@RoiEXLab
Copy link
Member Author

@DanVanAtta I have no idea, I presume some sort of whitespace? Windows doesn't seem to like files ending with regular whitespaces either apparently.

Can we fix this without nuking the file?

Maybe. It doesn't look like this file was actually ever working in the first place (because of the additional character), so this PR shouldn't change anything from a functional point of view. I initially tried just removing the whitespace, but this resulted in a build failure because it seems like there was some sort of issue with the parallelism this file was introducing, so I decided to remove the file entirely. If you have an idea how to fix the build failures, feel free to reintroduce the file.

Perhaps there is something we can add in .gitattributes?

I doubt it. The problem was the filename itself, not the actual contents of the file. A sort of similar problem happens when you commit 2 files with the same name but different casing on a UNIX-like system, and try to check it out on a windows system. However when this happens you can usually resolve it yourself, because git thinks you just removed one of the two files, because windows (by default) treats the filenames as synonyms. With some tricks you can just rename one of the files and you're fine. (NTFS is actually a case sensitive file system, but windows introduces the case insensitivity by default, it can be disabled in individual directories). But in this case it just stopped you from interacting with the repo entirely.

@DanVanAtta
Copy link
Member

It looks like the invalid character was simply a trailing space. I see that this is an invalid filename for windows.

Removing that space and the configuration did get picked up and the tests do run in parallel (a lot faster). Though, we do hit single-threadedness issues with the AI battle calculator insisting it run on one thread. This seems like a potential place for a massive performance boost if we can resolve that.

I marked the corresponding AI test to run with a single thread and it hardly buys a faster runtime (8 seconds vs 6.7 seconds, sometimes the same runtim @ 8 seconds).

I tried this config out in 'game-core' and it reduced test runtime down from 40s to 22s, but there were concurrency errors.

Bottom line, seems like the tests are not yet set up to run in parallel. Fixing that would unlock performance gains when running tests, but presumably large gains for the production code if we could run it in parallel.

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.

None yet

2 participants