search

replit / polygott

Base Docker image for the Repl.it evaluation server
https://replit.com
MIT License
395 stars 103 forks source link

Upgraded C++17 to C++20 #246

Closed alexapostolu closed 2 years ago

basicer commented 3 years ago

Clang 7 doesn't support C++20. You will need to update to a newer version of clang/llvm.

note: use 'c++98' or 'c++03' for 'ISO C++ 1998 with amendments' standard
note: use 'gnu++98' or 'gnu++03' for 'ISO C++ 1998 with amendments and GNU
      extensions' standard
note: use 'c++11' for 'ISO C++ 2011 with amendments' standard
note: use 'gnu++11' for 'ISO C++ 2011 with amendments and GNU extensions'
      standard
note: use 'c++14' for 'ISO C++ 2014 with amendments' standard
note: use 'gnu++14' for 'ISO C++ 2014 with amendments and GNU extensions'
      standard
note: use 'c++17' for 'ISO C++ 2017 with amendments' standard
note: use 'gnu++17' for 'ISO C++ 2017 with amendments and GNU extensions'
      standard
note: use 'c++2a' for 'Working draft for ISO C++ 2020' standard
note: use 'gnu++2a' for
      'Working draft for ISO C++ 2020 with GNU extensions' standard
alexapostolu commented 3 years ago

@basicer I added clang++12 to the Docker file, so C++20 should be supported now, yet I'm still getting CI errors. For the artifacts_freshness test, I get this error:

There are changes to out/ that need to be checked in.

Did I forget to check in clang++12 somehow?

basicer commented 3 years ago

@basicer I added clang++12 to the Docker file, so C++20 should be supported now, yet I'm still getting CI errors. For the artifacts_freshness test, I get this error:

There are changes to out/ that need to be checked in.

Did I forget to check in clang++12 somehow?

The new build system seems to requires you to check in the computed changes to some of the build files. I think you need to run make build/stamps/out and commit the changes in the out directory but im not 100% sure! The new build system is a bit complex so it can go at warp speed.

lhchavez commented 3 years ago

@basicer I added clang++12 to the Docker file, so C++20 should be supported now, yet I'm still getting CI errors. For the artifacts_freshness test, I get this error:

There are changes to out/ that need to be checked in.

Did I forget to check in clang++12 somehow?

The new build system seems to requires you to check in the computed changes to some of the build files. I think you need to run make build/stamps/out and commit the changes in the out directory but im not 100% sure! The new build system is a bit complex so it can go at warp speed.

this is correct! i added a lil' blurb about it at the end of https://github.com/replit/polygott#build-and-run, but suggestions are welcome to make this more obvious!

The CI requires having up-to-date generated artifacts (the files in out/) committed to ensure a consistent Docker build environment. These will be refreshed by running the tests, or by running make -B build/stamps/out).

alexapostolu commented 3 years ago

The CI requires having up-to-date generated artifacts (the files in out/) committed to ensure a consistent Docker build environment. These will be refreshed by running the tests, or by running make -B build/stamps/out).

For the artifacts test, I had some troubles running the test make test-cpp on my own, so I instead ran make -B build/stamps/out. The test still failed though. I checked the CI, and was I supposed to run it without the -B flag like so: make build/stamps/out?

lhchavez commented 3 years ago

The CI requires having up-to-date generated artifacts (the files in out/) committed to ensure a consistent Docker build environment. These will be refreshed by running the tests, or by running make -B build/stamps/out).

For the artifacts test, I had some troubles running the test make test-cpp on my own, so I instead ran make -B build/stamps/out. The test still failed though. I checked the CI, and was I supposed to run it without the -B flag like so: make build/stamps/out?

oh, i think you're the first person to try to send a commit with Windows, which uses different end-of-line characters (\r\n instead of \n like all other OSs). you can try directly applying the patch that's in the CI output:

polygott> git apply <enter>
diff --git a/out/share/polygott/self-test.d/ballerina b/out/share/polygott/self-test.d/ballerina
index 20b185a..679d5ee 100755
--- a/out/share/polygott/self-test.d/ballerina
+++ b/out/share/polygott/self-test.d/ballerina
@@ -14,7 +14,7 @@ polygott-lang-setup -l 'ballerina'

 # ballerina:hello
 SUCCESS=true
-OUTPUT="$(echo 'aW1wb3J0IGJhbGxlcmluYS9pbzsNCnB1YmxpYyBmdW5jdGlvbiBtYWluKCkgew0KICAgIGlvOnByaW50bG4oIkhlbGxvLCBXb3JsZCEiKTsNCn0NCg==' | \
+OUTPUT="$(echo 'aW1wb3J0IGJhbGxlcmluYS9pbzsKcHVibGljIGZ1bmN0aW9uIG1haW4oKSB7CiAgICBpbzpwcmludGxuKCJIZWxsbywgV29ybGQhIik7Cn0K' | \
           base64 --decode | \
           run-project -s -l 'ballerina' \
        )" || SUCCESS=false
....

 # wren
<Ctrl+Z><Enter>
lhchavez commented 3 years ago

The CI requires having up-to-date generated artifacts (the files in out/) committed to ensure a consistent Docker build environment. These will be refreshed by running the tests, or by running make -B build/stamps/out).

For the artifacts test, I had some troubles running the test make test-cpp on my own, so I instead ran make -B build/stamps/out. The test still failed though. I checked the CI, and was I supposed to run it without the -B flag like so: make build/stamps/out?

oh, i think you're the first person to try to send a commit with Windows, which uses different end-of-line characters (\r\n instead of \n like all other OSs). you can try directly applying the patch that's in the CI output:

polygott> git apply <enter>
diff --git a/out/share/polygott/self-test.d/ballerina b/out/share/polygott/self-test.d/ballerina
index 20b185a..679d5ee 100755
--- a/out/share/polygott/self-test.d/ballerina
+++ b/out/share/polygott/self-test.d/ballerina
@@ -14,7 +14,7 @@ polygott-lang-setup -l 'ballerina'

 # ballerina:hello
 SUCCESS=true
-OUTPUT="$(echo 'aW1wb3J0IGJhbGxlcmluYS9pbzsNCnB1YmxpYyBmdW5jdGlvbiBtYWluKCkgew0KICAgIGlvOnByaW50bG4oIkhlbGxvLCBXb3JsZCEiKTsNCn0NCg==' | \
+OUTPUT="$(echo 'aW1wb3J0IGJhbGxlcmluYS9pbzsKcHVibGljIGZ1bmN0aW9uIG1haW4oKSB7CiAgICBpbzpwcmludGxuKCJIZWxsbywgV29ybGQhIik7Cn0K' | \
           base64 --decode | \
           run-project -s -l 'ballerina' \
        )" || SUCCESS=false
....

 # wren
<Ctrl+Z><Enter>

merged a change to prevent this issue, so if you merge the master branch in this PR, the problem should go away!

ghost commented 3 years ago

Clang is just a language compiler, it doesn't carry any languages' standard libraries. They're independent and need to be updated separately.

For example, if not updated, anyone attempting to import a C++20 stdlib file will be met with errors regarding unresolved file paths or something of the sort.

I'm not too familiar with GCC, but the same situation may exist.

alexapostolu commented 3 years ago

I think I found a minor bug in the JSON schema when it's validating packages.

On the Ubuntu Bionic packages list, it says clang-10 requires the libstdc++6 standard library.

So following what @CrimsonCodes0 said, I added that package to the packages list for c.toml and ran make -B build/stamps/out; but I got a warning saying how the c.toml schema is invalid. I checked schema.json, and found this regex pattern for the packages: ^([a-z0-9.=-]+)$. I think that pattern does not account for the character +, but that character is found in the package name libstdc++6.

Can this warning be ignored? Or does it have to be fixed before I commit the new package?

ALANVF commented 3 years ago

No you still need the + there. just need to add another + in the [...]

lhchavez commented 3 years ago

No you still need the + there. just need to add another + in the [...]

that's a red herring (although we still want to fix the + issue in another PR): we're already including libstdc++-8-dev (the package that installs the headers) in the image as a dependency of another package (which is why it's never mentioned explicitly anywhere). Bionic doesn't have a package for libstdc++-9-dev or beyond (which you need libstdc++-11-dev if you want full support for the whole language). this means that this change, as-is, will provide all of the core language features (like the spaceship operator), but most of the C++20 library features will be missing. that's still a net improvement, so let's go with that.

right now we're facing some limitations of the packages that Bionic can provide us, but in the next few weeks we may be able to provide a solution for that through nix.

alexapostolu commented 3 years ago

Yeah, it isn't a perfect fix, but clang-10 is still an improvement over clang-7. Are you guys considering merging it?

lhchavez commented 3 years ago

Yeah, it isn't a perfect fix, but clang-10 is still an improvement over clang-7. Are you guys considering merging it?

Yup, although CI is currently unhappy :'(

things are moving pretty fast in the new nix world (which may obsolete polygott altogether) so it may be easier to just go to nix at this point.

firefish111 commented 3 years ago

@DynamicSquid figjßłe←→¶ŧ←→¶ŧ←¥ ̛Ŧ&¥ ̛®&¥ ̛ĸ¶ŧĸ←ˀø¶e ̛ŁΩ& ̛ŁEŁĸˀŋđłĸŧˀł¶ẻŧĸðđŋĸłß¶nŧĸẻ¶ßĸøŋˀ↓øełŧˀ↓ø→łẻ¶đnŋđĸłŋnłðˀßđŧłĸˀßðe¶ĸßđ↓¢ĸ←ø↓¶ø”łø¢ĸħeæðŋĸħˀÐ&ˀ¶ŧĸ←ˀĸ←ˀĸŋˀħĸłeˀŧˀĸɠhĸłðˀŋĸħˀðđłˀ¶→øˀ¶ŧ←ˀłˀłĸđŋħĸđ←łĸˀ¶ŧ→←ˀ¶ĸŧˀłĸðđˀŋĸn¶ŧŋđ←ĸˀ¶eŧ¥&®¥˝ ̛®←ĸ¶ŧĸ←ˀĸł←ˀŧˀ←¶ŧĸ←ˀĸŋnħµdfghn,mgfhnd,fn←łˀ¶ee¶←ˀ¶e←ˀ→¶ŧ←→ø↓↓ø→←ø→e