[ONE-Dockerizing] onecc-docker using recent version only by morumoru · Pull Request #9722 · Samsung/ONE

morumoru · 2022-09-16T03:04:31Z

introduce onecc-docker using recent version only

Signed-off-by: Junsu Kim xofkdqkqh@naver.com

seanshpark · 2022-09-16T03:12:55Z

compiler/onecc-docker/Dockerfile

+RUN apt update -y && \ 
+	 apt install -y \ 
+	 wget && \ 
+	 apt clean 


Suggested change

RUN apt update -y && \

apt install -y \

wget && \

apt clean

RUN apt update -y && \

apt install -y wget && \

apt clean

please do not use TABs

I realized that this code don`t pass nacc format
I will check well before making pull request next time

seanshpark · 2022-09-16T03:14:47Z

compiler/onecc-docker/Dockerfile

+	 apt clean 
+RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb && \ 
+	 apt-get install -y ./one-compiler_1.20.0_amd64.deb 
+ENTRYPOINT ["onecc"]


please add new line here.

please check before posting PRs that you should not see these red circle.

seanshpark · 2022-09-16T03:16:13Z

compiler/onecc-docker/onecc-docker

@@ -0,0 +1,84 @@
+#!/usr/bin/env bash


it would be better to split this script and Docker files.
or can you explain the why Dockerfile and this file has close relation so that should be in one PR?

Yes. Dockerfile is not related with this script and I'll remove Dockerfile on next PR
We want to show people complete Dockerfile that this script will make but It was wrong.

We want to show people complete Dockerfile that this script will make but It was wrong.

In this case, you can use a draft PR. A Draft PR is not for merging, so you can show whole files you want. :)

jyoungyun

Please add usage message. :)

I have some trouble to run this script because of samsung network issue.
In this case, I got the below error message:

$ ./onecc-docker 
403
rate limit exceeded
we can't get recent tag

In error situation, it would be good to write a detail message to make it easier for users to understand rather than simply printing a log. :)

jyoungyun · 2022-09-16T03:11:29Z

compiler/onecc-docker/Dockerfile

@@ -0,0 +1,9 @@
+FROM ubuntu:18.04 


Suggested change

FROM ubuntu:18.04

# Copyright (c) 2022 Samsung Electronics Co., Ltd. All Rights Reserved

#

# Licensed under the Apache License, Version 2.0 (the "License");

# you may not use this file except in compliance with the License.

# You may obtain a copy of the License at

#

# http://www.apache.org/licenses/LICENSE-2.0

#

# Unless required by applicable law or agreed to in writing, software

# distributed under the License is distributed on an "AS IS" BASIS,

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

# See the License for the specific language governing permissions and

# limitations under the License.

FROM ubuntu:18.04

All codes should contain this samsung copyright. :)

jyoungyun · 2022-09-16T03:14:12Z

compiler/onecc-docker/Dockerfile

+RUN apt update -y && \ 
+	 apt install -y \ 
+	 wget && \ 
+	 apt clean 


Suggested change

RUN apt update -y && \

apt install -y \

wget && \

apt clean

RUN apt-get update && apt-get install -qqy --no-install-recommends \

wget \

&& rm -rf /var/lib/apt/lists/*

from https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run

jyoungyun · 2022-09-16T03:15:51Z

compiler/onecc-docker/Dockerfile

+RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb && \ 
+	 apt-get install -y ./one-compiler_1.20.0_amd64.deb 


Suggested change

RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb && \

apt-get install -y ./one-compiler_1.20.0_amd64.deb

RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb \

&& apt-get install -y ./one-compiler_1.20.0_amd64.deb \

&& rm -rf /var/lib/apt/lists/*

jyoungyun · 2022-09-16T03:16:43Z

compiler/onecc-docker/Dockerfile

+	 apt clean 
+RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb && \ 
+	 apt-get install -y ./one-compiler_1.20.0_amd64.deb 
+ENTRYPOINT ["onecc"]


Suggested change

ENTRYPOINT ["onecc"]

ENTRYPOINT ["onecc"]

jyoungyun · 2022-09-16T03:18:30Z

compiler/onecc-docker/onecc-docker

@@ -0,0 +1,84 @@
+#!/usr/bin/env bash
+''''exec "python3" "onecc-docker" "$@" #'''


Suggested change

''''exec "python3" "onecc-docker" "$@" #'''

''''exec "python3" "$0" "$@" #'''

jyoungyun · 2022-09-16T03:30:02Z

compiler/onecc-docker/onecc-docker

@@ -0,0 +1,84 @@
+#!/usr/bin/env bash


Please add samsung copyright.

jyoungyun · 2022-09-16T03:32:57Z

Before uploading a code, please check the code convention using ./nnas format ./compiler/onecc-docker. :)
I missed talking to you. :(

/cc @Samsung/ootpg

holawan · 2022-09-16T04:34:22Z

compiler/onecc-docker/onecc-docker

+def write_dockerfile(script_path,version) :
+
+    with open(f'{script_path}/Dockerfile', 'w', encoding='utf-8') as dockerfile: 
+        dockerfile.write('FROM ubuntu:18.04 \n\n')
+        dockerfile.write('RUN apt update -y && \ \n')
+        dockerfile.write('\t apt install -y \ \n')
+        dockerfile.write('\t wget && \ \n')
+        dockerfile.write('\t apt clean \n')
+        dockerfile.write(f'RUN wget https://github.com/Samsung/ONE/releases/download/{version}/one-compiler_{version}_amd64.deb && \ \n')
+        dockerfile.write(f'\t apt-get install -y ./one-compiler_{version}_amd64.deb \n')
+        dockerfile.write('ENTRYPOINT ["onecc"]')


@jyoungyun
Currently, we check the latest version of "ONE" with github api and dynamically create a Dockerfile. Is this all right? Or is it better to commit one Dockerfile statically and use it continuously?

This way is good to me.

Another way I thought was to create static Dockerfile with declaring version as a variable, and update the variable in bash command.

I think both ways are worth trying.

However, what about using the static Dockerfile with the fixed version like you implemented before?

You can just parameterize the version with --build-arg option. Please see https://docs.docker.com/engine/reference/commandline/build/#set-build-time-variables---build-arg for more detail.

Seunghui98 · 2022-09-16T05:06:57Z

@jyoungyun

Oh, thank you for code review. Is it better to commit push what has been modified through code review within the current PR #9722? Or should I put up a new PR?

jyoungyun · 2022-09-16T07:05:05Z

Oh, thank you for code review. Is it better to commit push what has been modified through code review within the current #9722? Or should I put up a new PR?

What about taking one PR is for onecc-docker and another PR is for Dockerfile ?

Seunghui98 · 2022-09-16T07:19:37Z

@jyoungyun

Oh, thank you for your good opinion. As you advised, I will separate it into two PRs and upload it.

mhs4670go · 2022-09-16T13:11:22Z

compiler/onecc-docker/onecc-docker

+    onecc_version = subprocess.check_output("command -v onecc > /dev/null && echo $(onecc -v)", shell=True ,encoding='utf-8')
+    installed_onecc_version = onecc_version.split(' ')[2]


What if users don't have any version of one-compiler installed?

I thought that command -v onecc will return empty when onecc is not installed
but when I tested now, subprocess return nonzero exit status so It makes error on that. I'll change my code to care about absent of one-compiler.

mhs4670go · 2022-09-16T13:13:40Z

compiler/onecc-docker/onecc-docker

+    onecc_version = subprocess.check_output("command -v onecc > /dev/null && echo $(onecc -v)", shell=True ,encoding='utf-8')
+    installed_onecc_version = onecc_version.split(' ')[2]
+
+    script_path = re.sub("\n", "", os.popen('pwd').read())


How about this one for better readability?

Suggested change

script_path = re.sub("\n", "", os.popen('pwd').read())

script_path = os.path.realpath(__file__)

mhs4670go · 2022-09-16T13:16:43Z

compiler/onecc-docker/onecc-docker

+        print(response.reason)
+        print("we can't get recent tag")
+        exit(1)
+    else :


You don't need else syntax because above if always run exit.

Suggested change

else :

mhs4670go · 2022-09-16T13:17:20Z

compiler/onecc-docker/onecc-docker

+        versions_json = json.loads(versions_str)
+        recent_version = versions_json[0]["name"]
+
+    if(recent_version == installed_onecc_version):


This if condition works in the python script?

Suggested change

if(recent_version == installed_onecc_version):

if recent_version == installed_onecc_version:

I missed to erase parparentheses on my if statement.
It works on python script when I tested, but I'll erase parparentheses

Ah, you're right it works. FYI, I think parentheses should be used only if it improves readability or you actually want to change the order of expression calculation.

mhs4670go · 2022-09-16T13:19:11Z

Do you have any plans for testing?

holawan · 2022-09-17T02:51:26Z

Do you have any plans for testing?

@mhs4670go

Our current test plan is as follows.

Test the viability of onecc-docker in various Linux environments (Red Hat, Ubuntu, CentOS, etc.)
Identify the docker version that will work as we intend (so that it can be used in as many versions as possible)
onecc related tests
- Perform tests related to onecc in tests under one-compiler
- Verify that the option to write to *.cfg, *.workflow.json works correctly

Our plan is not perfect.

@jyoungyun ,Can you give me a good opinion?

jyoungyun · 2022-09-19T02:25:07Z

@jyoungyun ,Can you give me a good opinion?

At first, I thought it would be difficult to implement the test due to lack of time. However, even if the amount of implementation is reduced, implementing test codes seem to be a more complete way to write code. :)

I wrote a draft code related to test for your convenience. I didn't test it yet because it takes a lot of time to compile at first. :) What about supporting test codes by referring #9731.

morumoru added the SSAFY label Sep 16, 2022

morumoru requested a review from jyoungyun September 16, 2022 03:04

morumoru added the DRAFT A draft issue or PR for sharing one's current working status and discussion. label Sep 16, 2022

Seunghui98 mentioned this pull request Sep 16, 2022

[ONE-Dockerizing] shell script related to onecc-docker #9721

Closed

seanshpark reviewed Sep 16, 2022

View reviewed changes

jyoungyun reviewed Sep 16, 2022

View reviewed changes

holawan reviewed Sep 16, 2022

View reviewed changes

holawan mentioned this pull request Sep 16, 2022

[ONE_Dockerizing] Discussion of Docker Image Shrinking Process #9725

Closed

holawan assigned morumoru, Seunghui98 and holawan Sep 16, 2022

mhs4670go reviewed Sep 16, 2022

View reviewed changes

This was referenced Sep 19, 2022

[ONE-Dockerizing] onecc-docker latest version build #9736

Closed

[ONE-Dockerizing] Scripts that can run onecc using docker containers #9737

Closed

morumoru marked this pull request as draft September 20, 2022 01:47

morumoru closed this Sep 27, 2022

morumoru force-pushed the master branch from 4048c1a to 4b33246 Compare September 27, 2022 01:45

-FROM ubuntu:18.04
+# Copyright (c) 2022 Samsung Electronics Co., Ltd. All Rights Reserved
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+FROM ubuntu:18.04

		RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb && \
		apt-get install -y ./one-compiler_1.20.0_amd64.deb

-RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb && \
-	 apt-get install -y ./one-compiler_1.20.0_amd64.deb
+RUN wget https://github.com/Samsung/ONE/releases/download/1.20.0/one-compiler_1.20.0_amd64.deb \
+	 && apt-get install -y ./one-compiler_1.20.0_amd64.deb  \
+	 && rm -rf /var/lib/apt/lists/*

		@@ -0,0 +1,84 @@
		#!/usr/bin/env bash
		''''exec "python3" "onecc-docker" "$@" #'''

	''''exec "python3" "onecc-docker" "$@" #'''
	''''exec "python3" "$0" "$@" #'''

		onecc_version = subprocess.check_output("command -v onecc > /dev/null && echo $(onecc -v)", shell=True ,encoding='utf-8')
		installed_onecc_version = onecc_version.split(' ')[2]

	script_path = re.sub("\n", "", os.popen('pwd').read())
	script_path = os.path.realpath(__file__)

	if(recent_version == installed_onecc_version):
	if recent_version == installed_onecc_version:

Conversation

morumoru commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyoungyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyoungyun commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holawan Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Seunghui98 commented Sep 16, 2022

Uh oh!

jyoungyun commented Sep 16, 2022

Uh oh!

Seunghui98 commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morumoru Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhs4670go commented Sep 16, 2022

Uh oh!

holawan commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyoungyun commented Sep 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jyoungyun commented Sep 16, 2022 •

edited

Loading

holawan Sep 16, 2022 •

edited

Loading

morumoru Sep 16, 2022 •

edited

Loading

holawan commented Sep 17, 2022 •

edited

Loading