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 IO to OpenEducation methodology #75

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mihnea0Firoiu
Copy link

@Mihnea0Firoiu Mihnea0Firoiu commented Aug 5, 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

@github-actions github-actions bot added area/reading Update to reading content area/tasks Update to tasks kind/new New content / item labels Aug 5, 2024
@teodutu teodutu marked this pull request as ready for review August 5, 2024 15:53
@teodutu teodutu added the needs-rendering The PR makes changes to the website that need to be rendered label Aug 5, 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 Aug 11, 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.

Check my comments to see some easy first fixes and also why the build fails. Then when it passes I'll make a more in-depth review.

  • Move the task descriptions from the reading/ files to each task's README.md
  • Remove references to the common makefiles (.../common/makefile/*.mk). Copy their content to each makefile that includes them so the tasks don't depend on outside resources.

Copy link

Choose a reason for hiding this comment

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

Don't forget to add the task description to this README file.

@@ -0,0 +1,79 @@
/* SPDX-License-Identifier: BSD-3-Clause */
Copy link

Choose a reason for hiding this comment

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

Suggested change
/* SPDX-License-Identifier: BSD-3-Clause */
// SPDX-License-Identifier: BSD-3-Clause

Use // for SPDX tags in .c files and /* ... */ for .hs.

@@ -0,0 +1,15 @@
/* SPDX-License-Identifier: BSD-3-Clause */
Copy link

Choose a reason for hiding this comment

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

Suggested change
/* SPDX-License-Identifier: BSD-3-Clause */
// SPDX-License-Identifier: BSD-3-Clause

@@ -0,0 +1,74 @@
/* SPDX-License-Identifier: BSD-3-Clause */
Copy link

Choose a reason for hiding this comment

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

Suggested change
/* SPDX-License-Identifier: BSD-3-Clause */
// SPDX-License-Identifier: BSD-3-Clause

Comment on lines +1 to +2
BINARIES = file_operations directory_operations
include ../../../../../common/makefile/multiple.mk
Copy link

Choose a reason for hiding this comment

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

Remove refernces to global makefiles.

Comment on lines +1 to +2
BINARIES = send_fd_4 receive_pipe send_fifo receive_fifo send_unix_socket receive_unix_socket send_net_dgram_socket receive_net_dgram_socket
include ../../../../../common/makefile/multiple.mk
Copy link

Choose a reason for hiding this comment

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

Ditto: Remove references to external makefiles.

Comment on lines +32 to 35
student@os:~/.../lab/multiplex/support$ make
gcc -g -Wall -Wextra -I../../../../../common/makefile/.. -c -o epoll_echo_server.o epoll_echo_server.c
gcc -g -Wall -Wextra -I../../../../../common/makefile/.. -c -o ../../../../../common/makefile/../utils/log/log.o ../../../../../common/makefile/../utils/log/log.c
gcc -g -Wall -Wextra -I../../../../../common/makefile/.. -c -o ../../../../../common/utils/sock/sock_util.o ../../../../../common/utils/sock/sock_util.c
Copy link

Choose a reason for hiding this comment

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

After making the makefiles standalone, re-run these make commands and paste the new output here.

Comment on lines 107 to 109
2. Now, it is interesting to see how we can force libc to dump that internal buffer.
The most direct way is by using the `fflush()` library call, which is made for this exact purpose.
But we can be more subtle.
Copy link

Choose a reason for hiding this comment

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

Suggested change
2. Now, it is interesting to see how we can force libc to dump that internal buffer.
The most direct way is by using the `fflush()` library call, which is made for this exact purpose.
But we can be more subtle.
1. Now, it is interesting to see how we can force libc to dump that internal buffer.
The most direct way is by using the `fflush()` library call, which is made for this exact purpose.
But we can be more subtle.
Suggested change
2. Now, it is interesting to see how we can force libc to dump that internal buffer.
The most direct way is by using the `fflush()` library call, which is made for this exact purpose.
But we can be more subtle.
Add a `\n` in some of the strings printed in `buffering/support/printf_buffering.c`.
Place them wherever you want (at the beginning, end, or middle).
Recompile the code and observe its change in behaviour under `strace`.

You don't need to disable MD029 above. Remove the comment above that disables this rule and add a tab to each new line within the same numbered list item to keep the rendered numbering correct. Do the same for the list item above.

Comment on lines +110 to 114
Add a `\n` in some of the strings printed in `buffering/support/printf_buffering.c`.
Place them wherever you want (at the beginning, at the end, in the middle).
Recompile the code and observe its change in behaviour under `strace`.

<!-- markdownlint-enable MD029 -->
Copy link

Choose a reason for hiding this comment

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

Suggested change
Add a `\n` in some of the strings printed in `buffering/support/printf_buffering.c`.
Place them wherever you want (at the beginning, at the end, in the middle).
Recompile the code and observe its change in behaviour under `strace`.
<!-- markdownlint-enable MD029 -->
Add a `\n` in some of the strings printed in `buffering/support/printf_buffering.c`.
Place them wherever you want (at the beginning, end, or middle).
Recompile the code and observe its change in behaviour under `strace`.

Copy link

Choose a reason for hiding this comment

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

This link is broken. That's why the build fails.

@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 27, 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.

The build fails because chapters/io/arena/drills/tasks/mini-shell/ is missing its support/ folder which causes the Makefile symlink to point to an invalid path.

@@ -0,0 +1,19 @@
#! /bin/bash

Copy link

Choose a reason for hiding this comment

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

Suggested change
# SPDX-License-Identifier: BSD-3-Clause

Copy link

Choose a reason for hiding this comment

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

Remove the include and copy-paste the contents of the included makefile here so the task doesn't have outside dependencies.


IP = "127.0.0.1" # Loopback IP address (localhost)
MAX_MESSAGE_LENGTH = 1024
PORT = 5000 # Port to listen for data data on (non-privileged ports are > 1023)
Copy link

Choose a reason for hiding this comment

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

Suggested change
PORT = 5000 # Port to listen for data data on (non-privileged ports are > 1023)
PORT = 5000 # Port to listen for data (non-privileged ports are > 1023)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/reading Update to reading 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.

2 participants