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

README.md updates #343

Merged
merged 8 commits into from
Jun 5, 2017
Merged

README.md updates #343

merged 8 commits into from
Jun 5, 2017

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented May 12, 2017

This primarily restructures the “building” section, including some fixes. Also drops a few completed items from README.md:

  • Add Fedora instructions for installing go-md2man
  • Move documentation build instructions to the end, with a separate header
  • Create “Building {in a container,without a container}” subsections
  • Restructure the “Building without a container” version
  • Start all command examples with $
  • Drop finished work from TODO
  • Add a warning about in-container builds

Note the last commit: e.g. on Fedora, make build creates an Ubuntu binary which does not even start (./skopeo: error while loading shared libraries: libdevmapper.so.1.02.1: cannot open shared object file: No such file or directory). I expect it’s even worse on macOS.

Perhaps we might want to move the “Building without a container” subsection in front of the “Building in a container” subsection?

Alternatively we might also document make binary-static (which works around the Fedora problem, but not the macOS problem—and does not actually work now because the container does not install static libraries), making the instructions even more complex?

For now, this is leaving the ordering and instructions in this respect as is.


@hferentschik @duglin FYI

@runcom
Copy link
Member

runcom commented May 14, 2017

👍 @hferentschik would you take a look at this?

@mtrmac mtrmac force-pushed the readme-updates branch 2 times, most recently from 943b0c1 to 3f24e51 Compare May 24, 2017 20:43
README.md Outdated
```sh
$ sudo apt-get install go-md2man
$ make binary # Or (make all) to also build documentation, see below.
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs make more sense now, but when I actually do run make binary I get:

....
Step 4 : WORKDIR /src/github.com/projectatomic/skopeo
 ---> Running in bf527540083c
 ---> f817788091bf
Removing intermediate container bf527540083c
Successfully built f817788091bf
docker run --rm --security-opt label:disable -v $(pwd):/src/github.com/projectatomic/skopeo \
		skopeobuildimage make binary-local  BUILDTAGS='btrfs_noversion libdm_no_deferred_remove     containers_image_openpgp'
CGO_ENABLED=1 GOARCH=amd64 GOOS=darwin go build -ldflags "-X     main.gitCommit=07c798ff82f82a68571b03eb9be3469baf483d93" -gcflags "" -tags "btrfs_noversion     libdm_no_deferred_remove containers_image_openpgp" -o skopeo ./cmd/skopeo
# net
could not determine kind of name for C.AI_MASK
Makefile:60: recipe for target 'binary-local' failed
make: *** [binary-local] Error 2
make: *** [binary] Error 2    

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’m afraid I have no idea; random googling suggests that this is a problem when cross-compiling from Linux to macOS.

CGO_ENABLED=1 GOARCH=amd64 GOOS=darwin go build …

How did the CGO_ENABLED=1 GOARCH=amd64 GOOS=darwin get there? I can’t see anything like that in the Makefile. Is go doing some magic autodetection, or is that a local modification?

README.md Outdated
```
To build the `skopeo` binary you need at least Go 1.5 because it uses the latest `GO15VENDOREXPERIMENT` flag. Also, make sure to clone the repository in your `GOPATH` - otherwise compilation fails.
(Note that the created executable is a Linux executable, and depends on dynamic libraries which may only be available only in a container of a similar Linux distribution.)
Copy link
Contributor

Choose a reason for hiding this comment

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

So what is the long term vision for skopeo? Will it always be a Linux CLI tool? There is not intention to make it work on other OS types?

Copy link
Member

Choose a reason for hiding this comment

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

I think we would be open to pull requests that would allow it to run on other OS. But most of the people working on it are primarily Linux

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This paragraph only applies to the “building in a container” section; make binary-local should work just fine (though, as you can see, because we don’t have a working CI for macOS, I’m afraid it’s too easy to break it without noticing).

I’ve updated the “building in/without a container” sections to make the trade-offs clearer, and on balance it is probably better to start with the “without a container” alternative (the build instructions are longer, but much more broadly useful), so I have moved it before the in-container one.

README.md Outdated
$ git clone https://github.com/projectatomic/skopeo $GOPATH/src/github.com/projectatomic/skopeo
$ cd $GOPATH/src/github.com/projectatomic/skopeo && make all
Fedora$ dnf install gpgme-devel libassuan-devel
macOS$ brew install gpgme
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth to document how to compile without gpgme? containers/image has the containers_image_openpgp build flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that flag should work as well. Do you think it is useful enough to list it in here as another option, instead of just telling the users to install gpgme? I’m worried that the more options and variants are in the README, the more users are likely to just give up.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Jun 2, 2017

Added a commit which significantly expands the section on handling containers/image updates. @giuseppe does that make sense?

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

thanks for the additional info, it makes sense. Just noted inline a couple of minor typos while reading it.

README.md Outdated
@@ -184,10 +184,20 @@ In order to update an existing dependency:
- update the relevant dependency line in `vendor.conf`
- run `vndr github.com/pkg/errors`

In order to test out new PRs from [containers/image](https://github.com/containers/image) to not break `skopeo`:
When new PRs for [containers/image](https://github.com/containers/image) break `skopeo` (i.e. `contaners/image` tests fail in `make test-skopeo`):
Copy link
Member

Choose a reason for hiding this comment

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

s|contaners|containers|

README.md Outdated
- update `vendor.conf`. Find out the `containers/image` dependency; update it to vendor from your own branch and your own repository fork (e.g. `github.com/containers/image my-branch https://github.com/runcom/image`)
- run `vndr github.com/containers/image`
- make any other necessary changes in the skopeo repo (e.g. add other dependencies now requried by `containers/image`, or update skopeo for changed `containers/image` API)
Copy link
Member

Choose a reason for hiding this comment

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

s|requried|required|

It would be nice to have macOS instructions as well.
We want to start with the Go 1.5 dependency and build/checkout
instructions.

Also create a separate subsection, to match the future “Building
in/without a container” subsections
To make it clearer that the two are alternatives.

Document that a docker command is needed for the in-container build.

Also move the “checkout in $GOPATH” warning into the “without a
container” section, where it belongs.
Consolidate the Fedora and macOS instructions to prevent duplication,
and to suggest using $GOPATH for both.

Start with installing dependencies.
That has mostly been the case, with one outlier.  Fix it.
- We now have the docker-archive: transport
- Integration tests with built registries also exist
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jun 5, 2017

Thanks, typos updated.

@runcom Could you take another look, please?

@runcom
Copy link
Member

runcom commented Jun 5, 2017

@mtrmac looks great, thanks a lot

@mtrmac mtrmac merged commit 87a36f9 into containers:master Jun 5, 2017
@mtrmac mtrmac deleted the readme-updates branch June 5, 2017 15:50
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants