Build Failing because of auto about orbiter

[dimitry-ishenko]

It's because the standard was downgraded to c++17 (c4c7be5):

https://github.com/orbitersim/orbiter/blame/main/CMakeLists.txt#L9

and auto function parameters require c++20.

[jarmonik]

As far as I can tell it's NOT been downgraded. It was raised to C++20 without authorization and some commits been made that require C++20. I would hope to focus on development instead of playing with the Git.

[jarmonik]

The GitHub Build checks didn't fail a week ago, so what changed. Visual Studio 2019 builds/compiles the code without issues, so, the problem seems to be GitHub related.

[n7275]

I think the root cause of this is: some pull requests waited a long time for merge and became de-synced from upstream changes. The build checks should have alerted us to this issue, but they didn't. I think we need to pull, build and test each PR before merge. The C++20 change was 5 months ago, was on a single PR and was merged through normal approval channels. Gondos std::fs change depended on it, this is what broke things. We either need to move to 20 or revert this and leave it on hold for now. I spent some time reading old forum posts, github emails and discord messages. As far as I can tell and what I remember, we decided to hold off on major API breaking changes at least until we make one "release". But just updating the standard shouldn't break anything? More thoughts later when I've had more coffee...

…

On Thu, Nov 23, 2023, 04:16 jarmonik ***@***.***> wrote: The build didn't fail a week ago, so what changed. — Reply to this email directly, view it on GitHub <#389 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAYP34RCTYOCRZZPZCVGFDYF4H6JAVCNFSM6AAAAAA7VVF3DCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRUGA2DEMJXGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jarmonik]

I spent some time reading old forum posts, github emails and discord messages. As far as I can tell and what I remember, we decided to hold off on major API breaking changes at least until we make one "release". But just updating the standard shouldn't break anything?

Yes, we are supposed to release one more Orbiter with DirectX 7 and then we can remove the DX7 parts and cleanup some dirty hacks we have needed to keep the D3D9Client working. After this is done I am still expecting the API to be binary compatible with existing add-ons. So, nothing major is expected to happen. There is no point to turn the API upside-down just in a heck of it. I am no mind-reader but I guess that most developers would appreciate stability regarding the API

[dimitry-ishenko]

As far as I can tell it's NOT been downgraded.

It was with this commit: c4c7be5

It was raised to C++20 without authorization

Not sure what you mean. It was merged by @Xyon whom is an authorized committer as far as I can tell.

The GitHub Build checks didn't fail a week ago, so what changed. Visual Studio 2019 builds/compiles the code without issues, so, the problem seems to be GitHub related.

It's nothing to do with GitHub. As a test, I've switched the standard back to c++20 and it built just fine.

See this: 345483e and the result:

https://github.com/dimitry-ishenko-orbiter/orbiter/actions/runs/6959529860

[jarmonik]

Not sure what you mean. It was merged by @Xyon whom is an authorized committer as far as I can tell.

Yes he/she is and I am not blaming him. When I merge someones pull request I usually try to trust that contributor knows whats he's doing and I don't necessarily look through every line of code in great detail. I have merged some bad pulls in a past and I have spend days to debug and fix them afterwards, so, I am little more careful now on.

Decisions regarding API compatibility, build environment or something that impacts on everybody else's work should be discussed before making them just as a good habit.

[dimitry-ishenko]

Dimitry: I agree and they have been discussed in the forums and on discord. I am sorry you were not there.

Jarmonik: Sorry about that, I just don't have time to be everywhere and follow every discussion in detail. I would prefer receiving a link into a post that would summarize the issue at hand via PM through Orbiter Forum.

[jarmonik]

And to be clear, the reason why I proposed it was because I wanted to replace a lot of Windows-specific formatting code with the standard formatting library, which was added in c++20.

Well, I suppose that wouldn't hurt. What is this windows specific formatting. I searched the code and found nothing else than printf, scanf and std::strstream like formatting.

[n7275]

I think specifically std::format. We've encountered some annoying things in NASSP code, where we've come to rely on windows stuff like scanf_s() and not having strncasecmp

[jarmonik]

I think specifically std::format. We've encountered some annoying things in NASSP code, where we've come to rely on windows stuff like scanf_s() and not having strncasecmp

Well... As far as I can tell scanf_s is C++11 stuff not windows specific. https://en.cppreference.com/w/c/io/fscanf

[jarmonik]

If developers want to raise the level to C++20 then I have no reason to hold it back. EDIT: There are also discussion about C++ level over here and C++17 also receives some support. #375

[jarmonik]

What did just happen ??? I was replaying to Dimitry's post and it got replaced by my reply. That's odd.

[n7275]

What did just happen ??? I was replaying to Dimitry's post and it got replaced by my reply. That's odd.

That's odd. I've had github do things like that in the past though.

Digging into the build errors a bit more:

When I set the standard to CPP+20, I still have this line causing an error:

orbiter/Src/Orbiter/Psys.cpp

Line 230 in f9232ec

ifstream ulf(entry);

I checked out the branch https://github.com/TheGondos/orbiter/tree/filesystem which was recently merged on #374 and without changing anything, that branch fails to build.

If I checkout main, and revert acb9135 I get code that builds (ad runs). @TheGondos I think we need your help to get your filesystem branch updated so it can be merged. I'm presuming it was working on your end in July? It looks like a bunch of good changes and standardizations.

I think we need to revert acb9135 temporarily until we can figure out a fix. It's probably something simple. I am happy to make a pull request for this.

[dimitry-ishenko]

And to be clear, the reason why I proposed it was because I wanted to replace a lot of Windows-specific formatting code with the standard formatting library, which was added in c++20.

Well, I suppose that wouldn't hurt. What is this windows specific formatting. I searched the code and found nothing else than printf, scanf and std::strstream like formatting.

Honestly, I don't remember anymore, but in the issue you've linked (#375) I've laid out things I wanted to achieve and my reasoning.