Skip to content

Integrate atomic exercise as a final task in race exercise. #533

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

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

chavid
Copy link
Contributor

@chavid chavid commented Sep 29, 2024

The two exercises start from the same code, and go strongly together. I suggest we absorb atomic into race, as a final step.

@chavid chavid requested review from hageboeck and sponce September 29, 2024 20:13
Copy link
Contributor

@sponce sponce left a comment

Choose a reason for hiding this comment

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

The tests are failing because the top level CMakeLists.txt for exercises still tries to compile the subdirectory called atomic. I've fixed it but could not puch to your branch. Thus I've created #534 to test it, but you may want to fix your version.

Otherwise all fine for me.

@sponce
Copy link
Contributor

sponce commented Sep 30, 2024

Similarly, the atomic repo was linked to in ExerciseSchedule_AdvancedCourse.md file. Also fixedin #534

@sponce
Copy link
Contributor

sponce commented Sep 30, 2024

and also the CheatSheet. Now also fixed

@sponce
Copy link
Contributor

sponce commented Sep 30, 2024

There is now on top a merge conflict (fixed in #524) but on top tests are failing. The atomic ones, it's kind of normal as the ci-test has a list of tests harcoded. So I've removed the atomic one, but I do not manage to convince gitlab to run the new version. If you have an idea... Now there is also a failure for the race test : the CMakeLists.txt uses solution1 and solution2 targets without declaring them it seems.

@sponce
Copy link
Contributor

sponce commented Sep 30, 2024

Looks much better. Can you please rebase on master and force push the result ? We try to have a linear history.

@chavid
Copy link
Contributor Author

chavid commented Sep 30, 2024

@sponce, j'ai fini par en venir à bout.
@hageboeck, des remarques ?

@chavid
Copy link
Contributor Author

chavid commented Sep 30, 2024

J'ai essayé de rebaser... c'est bon ?

@chavid
Copy link
Contributor Author

chavid commented Sep 30, 2024

@sponce , it seems rebasing brought me a dead link in the clang-format section of the CheatSheet...

@sponce
Copy link
Contributor

sponce commented Sep 30, 2024

The dead link was a false positive (basically the llvm site must have been in troubles when the check was done). On the other hand, git still says "This branch cannot be rebased due to conflicts". Do you want me to try it ?

@chavid
Copy link
Contributor Author

chavid commented Oct 1, 2024

At last, all checks have passed !

I got several times to put back the section "condition_variables" in ExerciceSchedule_AdvancedCourse.md.
Where you wanting to remove it ?

@sponce
Copy link
Contributor

sponce commented Oct 1, 2024

I did not touch condition_variables recently, but probably someone did. The problem was that you've merged master into your branch at some stage. And thus the rebase was reapplying some master changes on itself. This may conflict if other changes were done afterward. Anyway, all ready, I'm merging

@sponce sponce merged commit 5cf64e7 into hsf-training:master Oct 1, 2024
86 checks passed
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