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

Revert "[build] don't require passwordless sudo (Closes #11292)" #12517

Closed
wants to merge 1 commit into from

Conversation

qiluo-msft
Copy link
Collaborator

Reverts #11417

This docker image alpine is unnecessary dependency.

@qiluo-msft
Copy link
Collaborator Author

@jusherma Could you help review?

Copy link
Contributor

@jusherma jusherma left a comment

Choose a reason for hiding this comment

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

This is reverting a change which fixed a build issue (#11292). This should not be reverted

@kv-y
Copy link
Contributor

kv-y commented Oct 26, 2022

This is reverting a change which fixed a build issue (#11292). This should not be reverted

Current code also doesn't work. It does nothing.

  1. Word shell is missed
  2. You can't remove mounted directory inside a container. Try and you get Resoure busy error.
    Possible solution is mount parent directory of $(DOCKER_ROOT).
  3. Instead of alpine probably better to use debian:$(BLDENV) container.

@@ -241,8 +241,7 @@ endif
DOCKER_LOCKFILE_SAVE := $(DOCKER_LOCKDIR)/docker_save.lock
$(shell mkdir -m 0777 -p $(DOCKER_LOCKDIR))
$(shell [ -f $(DOCKER_LOCKFILE_SAVE) ] || (touch $(DOCKER_LOCKFILE_SAVE) && chmod 0777 $(DOCKER_LOCKFILE_SAVE)))
$(docker run --rm -v $(DOCKER_ROOT)\:/mount alpine sh -c 'rm -rf /mount/')
$(mkdir -p $(DOCKER_ROOT))
$(shell sudo rm -rf $(DOCKER_ROOT) && mkdir -p $(DOCKER_ROOT))
Copy link
Contributor

@kv-y kv-y Oct 27, 2022

Choose a reason for hiding this comment

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

Suggested change
$(shell sudo rm -rf $(DOCKER_ROOT) && mkdir -p $(DOCKER_ROOT))
$(shell [ -d $(DOCKER_ROOT) ] && \
docker run --rm -v $(DOCKER_ROOT)/..\:/mount $(DEFAULT_CONTAINER_REGISTRY)debian:buster \
sh -c 'rm -rf /mount/$$(basename "$(DOCKER_ROOT)")')
  1. debian:buster because we already use it for DOCKER_MACHINE in Makefile.work, but maybe need a variable for default release name.
  2. We mount a parent directory because we can't remove bind mounted directory itself.
  3. I think we don't need mkdir command. If $(DOCKER_ROOT) doesn't exist then docker creates it (https://docs.docker.com/storage/bind-mounts/).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Konstantin. I not sure if $(shell ) is needed. My understanding is that it tells make to treat the command's output as if it were part of the makefile (https://www.gnu.org/software/make/manual/html_node/Shell-Function.html)

But if the above works, I'm not too particular about how it works. My only requirement is that sudo isn't used as that breaks the builds for us

Copy link
Contributor

Choose a reason for hiding this comment

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

@xumia
Could you please review this.
You added this feature in #11111
But side effect of your commit was requiring sudo for each make.

There was a fix for this in #11417.
But fix doesn't work correctly because:

  1. word shell was missed
  2. It's not possible to remove bind-mounted directory itself
    So at this moment fsroot directories are not removed when you do make.

Also this fix has some drawbacks:

  1. Docker image alpine is unnecessary dependency. We can use debian:buster instead (we already use it for DOCKER_MACHINE detection)
  2. We don't have to remove fsroot directory and run docker container for this every time (e.g. when we use a native docker)
  3. We don't have to recreate this directory because docker does it anyway (if we use -v option).

I can do a pull request instead of suggestion. But in this case it will take some time because I haven't signed CCLA yet.

@qiluo-msft qiluo-msft force-pushed the revert-11417-jusherma/sudo-rm-bug branch from 9514fdd to e83ff99 Compare November 14, 2022 04:43
@qiluo-msft
Copy link
Collaborator Author

Close in favor of #15132

@qiluo-msft qiluo-msft closed this May 18, 2023
@qiluo-msft qiluo-msft deleted the revert-11417-jusherma/sudo-rm-bug branch May 18, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants