Skip to content

Upgrade to xeus 5 #127

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 3, 2024
Merged

Upgrade to xeus 5 #127

merged 1 commit into from
Jun 3, 2024

Conversation

anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented May 29, 2024 •
edited
Loading

Fixes #108
Closes #110

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491 anutosh491 changed the title Update to xeus 5 Upgrade to xeus 5 May 29, 2024
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev vgvassilev requested review from JohanMabille and alexander-penev and removed request for JohanMabille May 29, 2024 14:27
@mcbarton
Copy link
Collaborator

@anutosh491 maybe link this to the issues about removing the xtl dependency and update the readme about depenencies on the main

@mcbarton
Copy link
Collaborator

@anutosh491 The failing test is the one which loads the image as it makes use of xtl when it loads the xtl header
#include "xtl/xbase64.hpp"
You'll also need to change this in the example notebook.

alexander-penev

This comment was marked as outdated.

Sign in to view

alexander-penev
alexander-penev requested changes May 30, 2024
View reviewed changes
Copy link
Collaborator

@alexander-penev alexander-penev left a comment

Choose a reason for hiding this comment

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

Yes, everything looks very good, only the test crashes because of the removed xtl. If the test works out to not use the now removed xtl, the PR can be merged.

@anutosh491
Copy link
Collaborator Author

as it makes use of xtl when it loads the xtl header

Ahh I overlooked this. Thanks for the debug :)

@mcbarton
Copy link
Collaborator

@anutosh491 will this also fix #54 since its related to xtl?

@anutosh491
Copy link
Collaborator Author

Hmm, let me think through it. I think it might just solve that !

@anutosh491
Copy link
Collaborator Author

@anutosh491 will this also fix #54 since its related to xtl?

Well the 2 problems have some overlap but are a bit different. I think I have a way to tackle the issue, will solve it through a subsequent PR.

@mcbarton
Copy link
Collaborator

@anutosh491 xeus_REQUIRED_VERSION needs to be version 5.1.0 now

@anutosh491
Copy link
Collaborator Author

@anutosh491 xeus_REQUIRED_VERSION needs to be version 5.1.0 now

Yeah I think I would use xeus >= 5.0.0 in the environment. Just was debugging if we were able to fetch 5.1.0. So required version could be 5.0.0

mcbarton
mcbarton reviewed May 31, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

anutosh491
anutosh491 commented May 31, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491 anutosh491 force-pushed the xeus_5 branch 2 times, most recently from 0064438 to fcedf53 Compare May 31, 2024 11:41
@anutosh491
Copy link
Collaborator Author

cc @alexander-penev @vgvassilev @mcbarton
This is ready now !

@anutosh491
Copy link
Collaborator Author

Ahh okay 1 thing. I'll address the readme too :)

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

JohanMabille
JohanMabille reviewed May 31, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions GitHub Actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 79.41%. Comparing base (60fb73d) to head (df310a7).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   79.31%   79.41%   +0.10%     
==========================================
  Files          17       17              
  Lines         609      612       +3     
  Branches       59       59              
==========================================
+ Hits          483      486       +3     
  Misses        126      126
Files Coverage Δ
src/xinspect.hpp 69.49% <ø> (ø)
src/xinterpreter.cpp 88.67% <100.00%> (+0.07%) ⬆️
src/main.cpp 42.85% <66.66%> (+2.11%) ⬆️
Files Coverage Δ
src/xinspect.hpp 69.49% <ø> (ø)
src/xinterpreter.cpp 88.67% <100.00%> (+0.07%) ⬆️
src/main.cpp 42.85% <66.66%> (+2.11%) ⬆️

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 3, 2024

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491 anutosh491 force-pushed the xeus_5 branch 2 times, most recently from 94d9344 to 2bab746 Compare June 3, 2024 09:35
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 3, 2024

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491
Copy link
Collaborator Author

cc @vgvassilev ready !

@vgvassilev vgvassilev merged commit a2841c6 into compiler-research:main Jun 3, 2024
@anutosh491 anutosh491 deleted the xeus_5 branch June 3, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohanMabille JohanMabille JohanMabille approved these changes

@alexander-penev alexander-penev alexander-penev approved these changes

@mcbarton mcbarton mcbarton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop the dependency on xtl
6 participants
@anutosh491 @mcbarton @codecov-commenter @JohanMabille @alexander-penev @vgvassilev