-
Notifications
You must be signed in to change notification settings - Fork 91
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
SGX 2.10 & docker improvements #414
Conversation
3c813f0
to
b79695c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works pretty well and I like the new docker output! Some nits see below; works on my Mac.
Also, .travis.
still has some references to old sgx/ssl. I suggest to remove all that stuff from travis and just use docker. We could do this cleanup in this PR or create another one later. This is up to you.
Oops, completely forgot (have to update my checklist of what all to look at when updating sgx ...)
I'll fix it here. I will upgrade to sgx2.10 but as you correctly point out, we do not really need that as we all run in docker so will comment out any unnecessary steps (will save some time, though probably not too much as this is almost certainly cached by travis ..) |
20f036a
to
06bfb55
Compare
Ok, i started fixing it but turned out it was a bit more complicated than i thought and i run out of the time i gave me on it (i.e., time while i was attending another meeting ;-) and removed now everything so docker dev build is the only thing which will work. But as long as travis wouldn't have SGX HW support, this already should be as good as it gets in terms of testing ... |
@bvavala any chance to look into this already? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine.
Some orthogonal issues encountered:
- docker_buildkit 1 makes build hang behind a proxy
- demo fails with epid_linkable attestation type (it seems docker does not set the env variable properly)
- if you change attestation type, you must run a make clean, otherwise it fails, and there are no good indications why
config.mk
Outdated
PROJECT_NAME=fabric-private-chaincode | ||
# Docker related settings | ||
#-------------------------------------------------- | ||
export DOCKER_BUILDKIT ?= 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context: behind a proxy; DOCKER_BUILDKIT=1
Problem: while building base-rt, apt-get update... basez... hangs
Solution: default DOCKER_BUILDKIT=0
export DOCKER_BUILDKIT ?= 1 | |
export DOCKER_BUILDKIT ?= 0 |
* remove obsolete refernces to FPC_DOCKER_NAME_CMD (but leave corresponding source for now) * based on that, also moved utils in build list just before demo so docker stuff (only used stuff in utils right now) is built as late as possible as this could potentially be time-consuming * missing marbles example package in go.mod * removed obsolete references to sgxconfig Signed-off-by: Michael Steiner <[email protected]>
Seems the issue is with earlier versions of docker from ubuntu 18.04. doing
I've created an issue #426 for it, will address it in a separate PR ..
Not sure how we easily could fail gracefully. But will update docu as part of fixing #426 and mentioning the required |
…edup Signed-off-by: Michael Steiner <[email protected]>
…tion * This is mainly driven by a new (optional) DOCKER_QUIET_BUILD env variable * Also, given that we test in travis only through docker, remove the travis install steps Signed-off-by: Michael Steiner <[email protected]>
Signed-off-by: Michael Steiner <[email protected]>
98d6145
to
4e0bf3b
Compare
What this PR does / why we need it:
There are two main changes here
Additionally, there are a few small cleanup fixes i noticed as part of this effort.
What this PR does not yet do (will do it in seperate PR) is to refactor the use of uae_service to get rid of some legacy / epid centered libraries and header file (see a corresponding comment also in sgx psw install section in base-rt.
Which issue(s) this PR fixes:
This should address #411 (for now; once we close this, we can move forward #411 to MVP).
Special notes for your reviewer:
In case you have used
DOCKER_DEV_IMAGE_APT_ADD__PKGS
in$FPC_TOP/config.override.mk
: I replaced the accidental and confusing double_
with a single one, so you will have to change your config override file. However, you also get now more control that you can add packages to all images (viaDOCKER_BASE_RT_IMAGE_APT_ADD_PKGS
), to all images where fpc/fabric stuff is built (viaDOCKER_BASE_DEV_IMAGE_APT_ADD_PKGS) and yet further packages just for the dev image (via
DOCKER_DEV_IMAGE_APT_ADD_PKGS). Note also that i did remove packages which i knew would not be required for any default use of containers such as
vim`. So you might want to revisit your *_ADD_PKGS vars in light of that.Otherwise, as far as testing goes, there should be nothing special. It should work in SIM and HW mode and also when immediately runnning demo scripts without having run
make
first. (at least it did so for me :-)In case you start a fresh install on a vanilla machine and want to run in SGX HW mode, you also will have to do a
apt-get install libsgx-uae-service
which is not mentioned in the SGX SDK install doc. Note this package, though, will get obsolete once we merge PR #419 (but having it installed doesn't hurt).PS: i noticed now that all dockerfile show as completely new. I was pretty sure i always used git mv and checking my history the commands still show up. Not sure why the provenance was lost. Maybe too big changes in conjunction with git mv? Or the fact that i've done actually multiple git mv for dev (though not for base, now base-rt)? Sorry for that. Unfortunately, i don't see really a good way now to change that so you see a diff and the provenance shows that, e.g., base moved to base-rt and dev moved to base-dev ....
PS(2); if you haven't done a
make clobber
in the old setup, you will have obsolete images. Here the corresponding cleanup ...Does this PR introduce a user-facing changes and/or breaks backward compatability?: