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

Ubuntu 20.04 compatibility #6604

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Ubuntu 20.04 compatibility #6604

merged 1 commit into from
Feb 12, 2021

Conversation

ppenzin
Copy link
Member

@ppenzin ppenzin commented Feb 9, 2021

  • Drive by cleanup: Build all wabt files in the build directory

@ppenzin
Copy link
Member Author

ppenzin commented Feb 9, 2021

This is not all, there are other errors coming from both palrt and the main sources. I think this is due to newer Clang being more pedantic about C++ type compatibility.

I am not sure why the definitions in pal.h in needed, but I need to get through the entire build to find out.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 9, 2021

These defines are wrapped in a #ifndef PAL_STDCPP_COMPAT could we try defining PAL_STDCPP_COMPAT and seeing if that fixes things?

@ppenzin
Copy link
Member Author

ppenzin commented Feb 11, 2021

We do define that, but not for PAL sources. I tried adding this to PAL, but things did not go well with definitions in palinternal.h. Using C99 standard does not help with this issue either, as it is not getting passed to C++ files.

I was able to get through a number of template parameter errors, but now stuck back in PAL on malloc/free/realloc, which PAL reimplements.

I am also bumping Ubuntu CI version, so that we can see the progress. Also, given the situation, maybe we should test with both 18.04 and 20.04.

Copy link
Member Author

@ppenzin ppenzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to be consistent with these - the main goal is to simply give a different name to template parameters, so that they don't collide in situations like this:

template<class T>
class Foo {
  // Can't use the same name (T) here
  template<class T>
  friend class Bar;

  // ... and here
  template<class T>
  void Baz (T arg);
};

@ppenzin
Copy link
Member Author

ppenzin commented Feb 11, 2021

It was late at night and I changed Ubuntu image on copyright check job instead of builds. Also dropping PAL definitions change for now, will experiment and come up with something better.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 11, 2021

Interesting that Pal definitions are clashing with the Ubuntu stdlib. Could we just try deleting all the methods in pal that are in the stdlib?

@ppenzin ppenzin marked this pull request as ready for review February 12, 2021 05:27
@ppenzin ppenzin requested a review from rhuanjl February 12, 2021 05:27
@ppenzin
Copy link
Member Author

ppenzin commented Feb 12, 2021

If/when CI finishes it can be reviewed. I have added two more jobs for Ubuntu 18.04, not sure if they are needed, but thought it won't hurt since there are toolchain changes between those two versions.

Edit: never mind, style check failed, as I forgot to update the copyright. Hold on tight.

Drive by cleanup - build all wabt files in the build directory.

Disambiguate template parameter names. Overloading a template parameter
by a declaration (including in parameters to nested templates) is
treated as an error by Clang 10.

PAL math and memory API clean up (also for chakra-core#6404). Remove PAL
redefintions of system functions with incompatible exception
declarations. Remove externally visible redifinitions of math and memory
management functions. Delete corresponding math function
implementations, as they pretty much call their system counterparts.
Hide memory management functions, since they are still used from inside
of PAL.

[CI] Use Ubuntu 20.04 image for default Linux builds. Change default
Linux image to Ubuntu 20.04. Add two extra build jobs for Ubuntu 18.04.

Closes chakra-core#6600
@ppenzin
Copy link
Member Author

ppenzin commented Feb 12, 2021

Now as one big commit. There are mostly two things in it - template parameter disambiguation and PAL definition removal. It also adds two CI jobs for 18.04 and cleans up Wabt build set up.

@@ -33,18 +33,30 @@ jobs:
maxParallel: 6
matrix:
Linux.Debug:
image_name: 'ubuntu-18.04'
image_name: 'ubuntu-20.04'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we need the coverage - and let's take this in for now BUT I wonder if we could trim down slightly - maybe do debug on one platform and RelWithDebInfo on the other or something to reduce the CI run time.

@rhuanjl rhuanjl merged commit 3b3aa9b into chakra-core:master Feb 12, 2021
@ppenzin ppenzin deleted the ubuntu-20 branch February 13, 2021 05:40
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