Skip to content

Conversation

das7pad
Copy link
Contributor

@das7pad das7pad commented Apr 4, 2019

The - in front of the include statement causes make to ignore a missing file [1].

make docker will continue to work on the dev machine.

[1] https://www.gnu.org/software/make/manual/html_node/Include.html

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Apr 4, 2019
@sapk
Copy link
Member

sapk commented Apr 4, 2019

@das7pad What is the goal/use case ?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 4, 2019
@das7pad
Copy link
Contributor Author

das7pad commented Apr 4, 2019

There is no need to have a Makefile with docker commands in the docker container.

# docker run --rm -t --entrypoint=/bin/ls gitea/gitea:latest
Makefile  data      home      mnt       root      srv       usr
app       dev       lib       opt       run       sys       var
bin       etc       media     proc      sbin      tmp

# docker run --rm -t --entrypoint=/bin/ls gitea/gitea:no-make
app    data   etc    lib    mnt    proc   run    srv    tmp    var
bin    dev    home   media  opt    root   sbin   sys    usr

# docker images gitea/gitea
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
gitea/gitea         no-make             00d2a5c2ba6b        43 minutes ago      94.8MB
gitea/gitea         latest              f205743304c7        36 hours ago        94.9MB

@codecov-io
Copy link

codecov-io commented Apr 4, 2019

Codecov Report

Merging #6507 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6507      +/-   ##
==========================================
+ Coverage   41.13%   41.15%   +0.01%     
==========================================
  Files         425      425              
  Lines       58484    58484              
==========================================
+ Hits        24059    24067       +8     
+ Misses      31238    31231       -7     
+ Partials     3187     3186       -1
Impacted Files Coverage Δ
models/repo_list.go 67.89% <0%> (+1.05%) ⬆️
modules/log/event.go 65.98% <0%> (+1.52%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0081cd8...5c22bf8. Read the comment docs.

@sapk
Copy link
Member

sapk commented Apr 4, 2019

@das7pad Sorry, I was thinking that we only copy subfolder usr and etc. It must have changed or was on my todo list and think it was allready done.

Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

I am more in favor on only copying subfolder etc and usr than re-adding another file at root project folder. But still LGTM

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 4, 2019
@das7pad
Copy link
Contributor Author

das7pad commented Apr 6, 2019

I am fine with adding the two directories (/etc and /usr) explicit instead of re-adding the ignore file.

While preparing a patch I noticed that the Dockerfile commands COPY and ADD do not include the source directory. This line COPY docker/etc docker/usr / copies the contents of docker/etc and docker/usr into the root.
For example docker/etc/nsswitch.conf ends up as /nsswitch.conf.

I see two options at hand:

  • two layers for the copy calls
  • copy docker/etc and docker/usr into a new directory and copy this directory into the docker images root
    e.g. docker/etc -> docker/root/etc and the Dockerfile line COPY docker/root /

Let's see what the other maintainers think about it.

@lafriks
Copy link
Member

lafriks commented Apr 8, 2019

imho second option would be better

das7pad added 2 commits April 13, 2019 15:48
Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
ref 32b3253

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
techknowlogick and others added 2 commits April 13, 2019 20:51
# Conflicts:
#	docker/root/etc/s6/syslogd/finish
#	docker/root/etc/s6/syslogd/run
#	docker/root/etc/s6/syslogd/setup
@lafriks lafriks added this to the 1.9.0 milestone Apr 17, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 17, 2019
@techknowlogick techknowlogick merged commit dab38c3 into go-gitea:master May 6, 2019
@das7pad das7pad deleted the misc/drop-makefile branch May 6, 2019 19:25
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants