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

Add newer versions of EASTL and update install script. #1403

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

galorojo
Copy link

@galorojo galorojo commented Sep 1, 2024

There's 2 "interesting" things in this change (aside from the boring "just add new versions" part):

  • Adding recursive: false: EASTL suffered from a cyclical submodule dependency fiasco (see Circular submodule dependency causes recursive clones or submodule updates to fail electronicarts/EASTL#301), I actually don't know how this worked before. When I tried to do ce_install install eastl locally I'd hit the infinite recursion issue, this no longer occurs if we opt out of a recursive clone.
  • Adding some steps to run in after_stage_script: starting in version 3.21.23 EASTL moved away from submodule dependencies to potentially fix the circular dependency fiasco (see electronicarts/EASTL@c530255) and replaced them with CMake's FetchContent functionality. This script is an attempt to pull out the only non-test dependency (EABase) and put it in EASTL's include path so the parts of EASTL which do #include <EABase/foo.h> still work.

Tested running ce_install install eastl in a local instance of CE and things work as expected with all versions.

The corresponding change in compiler-explorer is compiler-explorer/compiler-explorer#6816

after_stage_script:
- mkdir build
- cd build
- /opt/compiler-explorer/cmake/bin/cmake ..
Copy link
Author

Choose a reason for hiding this comment

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

It is slightly unfortunate that we now run cmake even on the versions for which it is not necessary (the ones not using FetchContent), hopefully this isn't a big deal but I'm happy to hear about alternatives if anyone thinks of one.

Copy link
Member

Choose a reason for hiding this comment

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

Which versions? We can solve that by making a new section and only applying the after_stage_script to those. I'll try out in a branch and merge if I can get it to work.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @mattgodbolt,

We can solve that by making a new section and only applying the after_stage_script to those.

Thanks for your comment, I've changed the code a bit follow what benchmark is doing and this seems to work properly without doing any unnecessary work. I'm not sure if that's exactly what you were suggesting, or if there's a better way, let me know what you think.

bin/yaml/libraries.yaml Outdated Show resolved Hide resolved
@galorojo galorojo marked this pull request as ready for review September 1, 2024 06:35
@galorojo galorojo marked this pull request as draft September 2, 2024 06:10
@galorojo
Copy link
Author

galorojo commented Sep 2, 2024

Converting this to a draft given that EASTL might have to go back to submodules after all. (see electronicarts/EASTL#539). Apologies for the churn.

@mattgodbolt
Copy link
Member

Let me know when/if you're ready and I can help with the changes (ping me on Discord or here)

@galorojo
Copy link
Author

galorojo commented Oct 7, 2024

I still don't know what will happen with future versions of EASTL wrt going back to submodules, but either way I think this can be merged so we get up to date versions of EASTL working.

@galorojo galorojo marked this pull request as ready for review October 7, 2024 04:38
@partouf
Copy link
Member

partouf commented Oct 12, 2024

maybe it's better to switch over to building anyway (build_type: cmake) as they do have sources that need compiling, that was always the case but we never tried building eastl before

@galorojo
Copy link
Author

maybe it's better to switch over to building anyway (build_type: cmake) as they do have sources that need compiling, that was always the case but we never tried building eastl before

I think I might be missing a step in my local testing to get things built. I'll describe what I've tried and what I'm seeing to see if you spot what I'm missing:

To simplify, I'm only testing the version which isn't using submodules anymore and is using FetchContent instead. I've written this entry in the libraries.yaml file:

    eastl:
      build_type: cmake
      method: clone_branch
      recursive: false
      repo: electronicarts/EASTL
      targets:
      - 3.21.23
      type: github

Then I run ./bin/ce_install eastl with the following output:

2024-10-12 21:40:46,031 lib.ce_install  INFO     Creating multiprocessing pool with 8 workers
Installing libraries/c++/eastl 3.21.23
2024-10-12 21:40:50,215 libraries/c++/eastl 3.21.23 INFO     Cloning https://github.com/electronicarts/EASTL.git, branch: 3.21.23
HEAD is now at 7fadbf0 EASTL version 3.21.23 (#536)
HEAD is now at 7fadbf0 EASTL version 3.21.23 (#536)
2024-10-12 21:40:52,507 libraries/c++/eastl 3.21.23 INFO     Now at 7fadbf0da01e6f6e0e7038b1b34343a069b8fc51
2024-10-12 21:40:52,507 lib.installation_context INFO     Moving from staging (/opt/compiler-explorer/staging/02450cdb-03fa-472b-8dc4-ce17972bc9c9/libs/eastl/3.21.23) to final destination (/opt/compiler-explorer/libs/eastl/3.21.23)
2024-10-12 21:40:52,512 lib.ce_install  INFO     libraries/c++/eastl 3.21.23 installed OK
1 packages installed OK, 0 skipped, and 0 failed installation

Which gets the source of that version of eastl into my /opt/compiler-explorer/libs/eastl/ directory as expected, but nothing seems to get built, and in particular cmake isn't invoked so the dependent headers aren't fetched by FetchContent.

As a shot in the dark I tried ./bin/ce_install build eastl which got me this output:

2024-10-12 21:45:30,112 lib.installation_context INFO     Making uncached requests
Building libraries/c++/eastl 3.21.23 for all
0 packages built OK, 0 skipped, and 0 failed build

I suspect I'm missing something so that EASTL is actually built.

The other part is that even if I manage to build EASTL the include paths would still need fixing up (unless cmake informs the build command used by compiler explorer for cmake type libraries?). When EASTL was using submodules for dependencies we knew exactly where the dependent headers where located so we could add the dependent include paths to the compile command (see this as an example). But, now that it's using FetchContent the headers will be wherever CMake finds them or puts them (e.g. in a _deps subdirectory created when we run cmake) .

Admittedly, if we know how EASTL will be built and where Cmake will end up putting those files we can add the include directory to the command in the .properties folder as we were doing before for the submodule path, I'm not sure if that's better or worse than what I've done here.

You're right that EASTL has some source though so it'd be nice if we could get it building.

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.

3 participants