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

chapter/compute: Restructure repo according to updated methodology #74

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

AndreiMiga77
Copy link

@AndreiMiga77 AndreiMiga77 commented Aug 4, 2024

Prerequisite Checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Updated relevant documentation (if needed).

Description of changes

Restructure Compute chapter to match the OpenEdu methodology

  • Break arena into smaller sections and put it under sections
  • Make tasks self-contained
  • Generate support from solution whenever possible
  • Fix linters

@AndreiMiga77 AndreiMiga77 marked this pull request as draft August 4, 2024 18:09
@AndreiMiga77 AndreiMiga77 marked this pull request as ready for review August 4, 2024 18:27
@AndreiMiga77 AndreiMiga77 marked this pull request as draft August 4, 2024 18:28
@github-actions github-actions bot added area/tasks Update to tasks area/questions Update to questions content kind/new New content / item labels Aug 5, 2024
@teodutu teodutu marked this pull request as ready for review August 5, 2024 15:42
@teodutu teodutu added the needs-rendering The PR makes changes to the website that need to be rendered label Aug 5, 2024
Copy link

@teodutu teodutu left a comment

Choose a reason for hiding this comment

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

I made an initial, high-level pass. Fix my inline suggestions and the more general ones listed below (especially the one regarding building the site) and then I'll give this another look.

  • Add the following line at the end of each task: "If you're having difficulties solving this exercise, go through [this](link to the relevant section in reading/) reading material."
  • use git rm -rf content/chapters/compute to remove all remaining files in that folder once they're moved
  • Squash your commits into a single one because checkpatch runs on all commits every time. Further amend this single commit and git push -f upon every new review without adding additional commits.

chapters/compute/processes/guides/sum-array/README.md Outdated Show resolved Hide resolved
chapters/compute/threads/guides/sum-array/README.md Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
@AndreiMiga77 AndreiMiga77 force-pushed the compute_branch branch 4 times, most recently from eb27e7b to 691e5d0 Compare September 15, 2024 00:49
@teodutu teodutu added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Sep 18, 2024
Copy link

@teodutu teodutu left a comment

Choose a reason for hiding this comment

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

I gave you the fixes to config.yaml so the website builds properly. Apply them, then take care of my other suggestions.

  • Change this line [1] to - 'chapters/compute/**/*' so the PR label for topic/compute is assigned correctly.
  • Remove the arena/ folder. Extract all tasks from it and place them in whatever subchapter they fit best under drills/tasks/. You can prefix their names with "BONUS - " in config.yaml if they're more difficult.
  • Add the following line at the end of each task: "If you're having difficulties solving this exercise, go through [this](link to the relevant section in reading/) reading material."

[1]

- 'content/chapters/compute/**/*'

config.yaml Outdated Show resolved Hide resolved
@Alex-deVis Alex-deVis added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Oct 23, 2024
@Alex-deVis
Copy link

@teodutu, @razvand, @VladNastase I finished the directory structure change but the builder seems to fail (check action for logs). From what I understand, it complains that some media files are invalid. This is very weird since Software Stack/media, Data/media, and Compute/media are all copies of .view/media (generated with gen-view.py).

The image at "/build/docusaurus/SO/docs/Data/media/exec.svg" can't be read correctly. Please ensure it's a valid image.
unsupported file type: undefined (file: /build/docusaurus/SO/docs/Data/media/exec.svg)

The media paths are defined with extra: media/ in the config.yaml for each chapter. I will not be able to debug this week, so feel free to investigate, or leave suggestions on how to go about this.

@github-actions github-actions bot added the area/infra Update to infrastructure / scripts label Oct 23, 2024
@Alex-deVis
Copy link

It seems there were other issues I missed:

Warning: nedu_builder.plugins.docusaurus:STDERR: [WARNING] Duplicate routes found!

  • Attempting to create page at /operating-systems/74/Compute/, but a page already exists at this route.
  • Attempting to create page at /operating-systems/74/Data/, but a page already exists at this route.

This error makes no sense, I ran make clean build and did not change the Data chapter. We should look into this.

The real problem seems to be:

Error: Image docs/Data/media/100-percent-cpu.jpeg used in docs/Compute/lab6.md not found.
at async Promise.all (index 2)

It was either this or solving the dangling references in the .md files.

@Alex-deVis Alex-deVis added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Oct 24, 2024
@Alex-deVis Alex-deVis force-pushed the compute_branch branch 2 times, most recently from 0879431 to 6e25060 Compare October 24, 2024 08:47
Copy link

@Alex-deVis Alex-deVis force-pushed the compute_branch branch 3 times, most recently from 6cbd256 to e6fbe38 Compare October 27, 2024 07:55
@Alex-deVis Alex-deVis self-assigned this Oct 27, 2024
@teodutu teodutu added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Oct 27, 2024
Copy link

Copy link

@teodutu teodutu left a comment

Choose a reason for hiding this comment

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

I only found one rendering issue. Fix that and then I'm OK with this. And fix the merge conflict by moving the tasks to the beginning of each lab.

Copy jpeg media files to .view/media as they are used in Compute chapter.

Signed-off-by: Alex Apostolescu <[email protected]>
Relative links were checked only for questions, reading, and media
files. Handle tasks and guides in a similar way.

Signed-off-by: Alex Apostolescu <[email protected]>
@Alex-deVis Alex-deVis added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Oct 27, 2024
@Alex-deVis Alex-deVis added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Oct 27, 2024
Copy link

Comment on lines 20 to 25
1. Print the address and value of `var` in each thread.
See that they differ.

1. Modify the value of `var` in the `main()` function before calling `pthread_create()`.
Notice that the value doesn't propagate to the other threads.
This is because, upon creating a new thread, its TLS is initialised.
Copy link

Choose a reason for hiding this comment

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

Suggested change
1. Print the address and value of `var` in each thread.
See that they differ.
1. Modify the value of `var` in the `main()` function before calling `pthread_create()`.
Notice that the value doesn't propagate to the other threads.
This is because, upon creating a new thread, its TLS is initialised.
2. Print the address and value of `var` in each thread.
See that they differ.
3. Modify the value of `var` in the `main()` function before calling `pthread_create()`.
Notice that the value doesn't propagate to the other threads.
This is because, upon creating a new thread, its TLS is initialised.

Now because you removed the indent above, these list items aren't numbered properly because there's a break in the list. Just number them manually, I guess.

Choose a reason for hiding this comment

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

No, I think I just have to indent the subsequent phrases.

1. Print the address and value of `var` in each thread.
See that they differ.

should be written as:

1. Print the address and value of `var` in each thread.
  See that they differ.

Choose a reason for hiding this comment

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

Ignore this, I misread your comment.

Choose a reason for hiding this comment

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

OK, so the problem was that quizzes had the wrong indent. Seems like markdown expects 3 spaces indentation when you are under a numbered list.

- Break arena into smaller sections and put it under sections
- Make tasks self-contained
- Generate support from solution whenever possible

Signed-off-by: Andrei Miga <[email protected]>
Signed-off-by: Alex Apostolescu <[email protected]>
@Alex-deVis Alex-deVis added needs-rendering The PR makes changes to the website that need to be rendered and removed needs-rendering The PR makes changes to the website that need to be rendered labels Oct 27, 2024
Copy link

@teodutu teodutu merged commit 5a79c21 into cs-pub-ro:main Oct 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra Update to infrastructure / scripts area/questions Update to questions content area/tasks Update to tasks kind/new New content / item needs-rendering The PR makes changes to the website that need to be rendered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants