PLUS-POSTECH / soma

Cross-platform CTF problem container manager
Apache License 2.0
24 stars 3 forks source link

Use Path(Buf) instead of String when manipulating Path #98

Closed KSAlpha closed 5 years ago

KSAlpha commented 5 years ago

We decided to use String in some cases (like target_path of FileEntry) as Path is displayed differently per platform. However, manipulating path without an aid of Path(i.e. well tested and verified code) may result in an unexpected bug.

I suggest to use Path when Soma needs to manipulate it, and try to use path-slash crate to display it.

Qwaz commented 5 years ago

Related #53, #44

Qwaz commented 5 years ago

Converting backslash path to slash path can cause a problem on Windows depends on the use case. For instance, extended-path length only supports backslash as a delimiter. Thus, note that always converting a path to a slash path is not a solution.

KSAlpha commented 5 years ago

Yes, you are right. It is not a perfect solution to use slash path in all of the path related variables. But, I am sure that using slash path only in target_path (i.e displaying unix-understandable-path in windows platform) will not cause such problem.

When #80 is resolved, all copy instructions will be folded into COPY build/ / or ADD build/ / and this will prevent path of COPY {path} {target_path} making errors.

Qwaz commented 5 years ago

When #80 is resolved, all copy instructions will be folded into COPY build/ / or ADD build/ /

I thought it would be COPY build/ { work_dir }? (or maybe another configurable variable)

KSAlpha commented 5 years ago

I thought it would be COPY build/ { work_dir }? (or maybe another configurable variable)

It is COPY build/ / as the build directory will match to the root(/) of the image. For example, the files going into /home/prob_name will be copied into build/home/prob_name and will be properly added into /home/prob_name of the image.

Qwaz commented 5 years ago

I know what build directory will look like if we choose to use COPY build/ /. What is not described is why the build directory matches the root(/) of the image instead of the project home(/home/prob_name).

KSAlpha commented 5 years ago

According to #80, constructing build/ (i.e. constructing root file system of the image) is designed to be invisible to user, which is natural because it is an intermediate output. And all directories is always inclusive to root.

So, I think it is awkward to make build/ to match any other directories but root. I would like to know your reason why we should consider making build/ able to match any directory.

EDIT: I have updated #80 with an example for clarification.

Qwaz commented 5 years ago

I think the main confusion comes from the role of the working directory. I thought we do not support (or even prevent) putting files outside of the working directory.

Qwaz commented 5 years ago

Now I got your point. We already support putting files outside of the working directory with target_path attribute :/