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

engine: merge into the cli repository #1335

Closed
bassosimone opened this issue Feb 2, 2021 · 9 comments
Closed

engine: merge into the cli repository #1335

bassosimone opened this issue Feb 2, 2021 · 9 comments

Comments

@bassosimone
Copy link
Contributor

We are going to use Go 1.16's new feature that enables embedding assets. To this end, it would greatly simplify our life to have a single Go repository containing all the sources. This issue is about merging the cli and the engine repositories.

We could merge the cli into the engine, or the engine into the cli. Since the cli is tagging publicly available version numbers we will merge the engine into the cli.

Because of Go strict semantic versioning rules, we cannot export anything from the engine into the new repository, otherwise, we will need to break the import path at basically every new release of ooniprobe.

Therefore, we will basically vendor the engine codebase inside internal/engine. We still to hammer out a bunch of details including what to do of the existing engine repository, etc.

@bassosimone
Copy link
Contributor Author

bassosimone commented Feb 2, 2021

The first problem I identified is the way in which probe-cli uses semantic versioning. Consider this:

$ pwd
/tmp
$ go version
go version go1.15.6 linux/amd64
$ go get -v github.com/ooni/probe-cli/cmd/ooniprobe
go: found github.com/ooni/probe-cli/cmd/ooniprobe in github.com/ooni/probe-cli v0.3.0-rc.2
[...]
# github.com/ooni/probe-engine/measurementkit
g++: error: /usr/lib/libmaxminddb.a: No such file or directory
[...]
g++: error: /lib/libz.a: No such file or directory
[...]
github.com/ooni/probe-cli/internal/onboard
$ echo $?
2

This was happening because the import path github.com/ooni/probe-cli implied v0 or v1 as documented by Golang's wiki. See how the code above was building v0.3.0-rc.2.

The first step to fix this issue has been to tag v0.4.0. This is what happens now:

$ go version
go version go1.15.6 linux/amd64
$ go build -v github.com/ooni/probe-cli/cmd/ooniprobe
go: finding module for package github.com/ooni/probe-cli/cmd/ooniprobe
go: found github.com/ooni/probe-cli/cmd/ooniprobe in github.com/ooni/probe-cli v0.4.0
$ ./ooniprobe 
Please, use github.com/ooni/probe-cli/v3

This is quite an improvement. The binary we build fails and tells us what import path we should be using. (I can further improve that by listing the correct command to build and the full import path, but you get the idea.)

A second improvement is to make sure we can use a recent version of Go to build the correct binary. I implemented this change in ooni/probe-cli#200. This is what we can do now:

$ gotip version
go version devel +32e789f Mon Feb 1 21:38:01 2021 +0000 linux/amd64
$ gotip install -v github.com/ooni/probe-cli/v3/cmd/ooniprobe@master
$ `gotip env GOPATH`/bin/ooniprobe version
3.5.0-alpha

Here I needed to force @master because we don't have a tagged version of probe-cli that includes ooni/probe-cli#200.

BTW, of the two options available for releasing v2-or-higher Go modules, I chose the easier one. Just change go.mod and all the import paths. The more radical one, which calls for creating a subdirectory seems to create too much management complexity for me at this stage. We may reconsider for the future. But, for now, I don't see a need to support several major versions concurrently, so we should be good.

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
This is how I did it:

1. `git clone https://github.com/ooni/probe-engine internal/engine`

2. ```
(cd internal/engine && git describe --tags)
v0.23.0
```

3. `nvim go.mod` (merging `go.mod` with `internal/engine/go.mod`

4. `rm -rf internal/.git internal/engine/go.{mod,sum}`

5. `git add internal/engine`

6. `find . -type f -name \*.go -exec sed -i 's@/ooni/probe-engine@/ooni/probe-cli/v3/internal/engine@g' {} \;`

7. `go build ./...` (passes)

8. `go test -race ./...` (temporary failure on RiseupVPN)

9. `go mod tidy`

10. this commit message

Once this piece of work is done, we can build a new version of `ooniprobe` that
is using `internal/engine` directly. We need to do more work to ensure all the
other functionality in `probe-engine` (e.g. making mobile packages) are still WAI.

Part of ooni/probe#1335
@bassosimone
Copy link
Contributor Author

Now, the second step (ooni/probe-cli#201) is that of putting the probe-engine codebase into the probe-cli codebase. At this stage, we only care about being able to produce a working ooniprobe binary. Once this piece of work is done, there is extra work to ensure we can do with the probe-cli's repository everything we can do with probe-engine.

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
This is how I did it:

1. `git clone https://github.com/ooni/probe-engine internal/engine`

2. ```
(cd internal/engine && git describe --tags)
v0.23.0
```

3. `nvim go.mod` (merging `go.mod` with `internal/engine/go.mod`

4. `rm -rf internal/.git internal/engine/go.{mod,sum}`

5. `git add internal/engine`

6. `find . -type f -name \*.go -exec sed -i 's@/ooni/probe-engine@/ooni/probe-cli/v3/internal/engine@g' {} \;`

7. `go build ./...` (passes)

8. `go test -race ./...` (temporary failure on RiseupVPN)

9. `go mod tidy`

10. this commit message

Once this piece of work is done, we can build a new version of `ooniprobe` that
is using `internal/engine` directly. We need to do more work to ensure all the
other functionality in `probe-engine` (e.g. making mobile packages) are still WAI.

Part of ooni/probe#1335
@bassosimone
Copy link
Contributor Author

bassosimone commented Feb 2, 2021

Now, my task is to figure out which tests are failing and why. Exciting times 😬.

This work has now been done here: ooni/probe-cli#202. (For the records, the problem here is that some .gitignore files were too strict, so I didn't add everything I wanted to when I ran git add internal/engine.)

We're basically down to the same amount of failures we see in probe-engine.

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
* fix: add missing files causing tests to fail

See ooni/probe#1335 (comment)

* fix: toggle verbose so we better understand the tests output
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
Of course, also move the specific test checking whether we are
still able of building miniooni.

Part of ooni/probe#1335
@bassosimone
Copy link
Contributor Author

In ooni/probe-cli#203 we're adding support for building miniooni from probe-cli.

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
* refactor: build miniooni from toplevel

Of course, also move the specific test checking whether we are
still able of building miniooni.

Part of ooni/probe#1335

* build for current branch just to confirm

* fix: correct the path where linux/arm binary is

* okay, it works, we can remove the special rule
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
This diff is part of ooni/probe#1335.

We are moving more probe-engine workflows to toplevel.

The general idea here is to migrate all possible workflows and to
delete the ones that we cannot use in this repo (if any).
@bassosimone
Copy link
Contributor Author

In ooni/probe-cli#204 I'm migrating probe-engine workflows to probe-cli.

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
This diff is part of ooni/probe#1335.

We are moving more probe-engine workflows to toplevel.

The general idea here is to migrate all possible workflows and to
delete the ones that we cannot use in this repo (if any).
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 2, 2021
Part of ooni/probe#1335.

This seems also a good moment to move some packages out of the
engine, e.g., oonimkall. This package, for example, is a consumer
of the engine, so it makes sense it's not _inside_ it.
@bassosimone
Copy link
Contributor Author

The next step is automatically publishing an Android package. We're working on this in ooni/probe-cli#205.

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 3, 2021
* refactor: start building an Android package

Part of ooni/probe#1335.

This seems also a good moment to move some packages out of the
engine, e.g., oonimkall. This package, for example, is a consumer
of the engine, so it makes sense it's not _inside_ it.

* fix: committed some stuff I didn't need to commit

* fix: oonimkall needs to be public to build

The side effect is that we will probably need to bump the major
version number every time we change one of these APIs.

(We can also of course choose to violate the basic guidelines of Go
software, but I believe this is bad form.)

I have no problem in bumping the major quite frequently and in
any case this monorepo solution is convinving me more than continuing
to keep a split between engine and cli. The need to embed assets to
make the probe more reliable trumps the negative effects of having to
~frequently bump major because we expose a public API.

* fix: let's not forget about libooniffi

Honestly, I don't know what to do with this library. I added it
to provide a drop in replacement for MK but I have no idea whether
it's used and useful. I would not feel comfortable exposing it,
unlike oonimkall, since we're not using it.

It may be that the right thing to do here is just to delete the
package and reduce the amount of code we're maintaining?

* woops, we're still missing the publish android script

* fix(publish-android.bash): add proper API key

* ouch fix another place where the name changed
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 3, 2021
This is part of ooni/probe#1335. We also need
to think whether we wanna keep libminiooni and miniooni separated.

The previous use case for having a top-level libminiooni was that of
enabling others to integrate miniooni into other binaries.

This was usegul when studying internet censorship in Spain in May 2020.

I am wondering whether we should be keeping this complexity. I am not
sure about this and probably we should be killing it.

(In any case, reducing complexity is not the objective of this diff,
since I would like instead to move things around with minimal changes
and make sure we have a ~good repository organization here.)
@bassosimone
Copy link
Contributor Author

More refactoring underway in ooni/probe-cli#206. It's a set of minor changes and movements after we merged the two codebases. This is ~easy and quite useful to help me identify optional complexity to kill.

bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 3, 2021
* doc: ensure all top dirs have an explanatory README

This makes the repository a lil bit nicer to newcomers.

Part of ooni/probe#1335

* fix: re-run bindata to embed the README

The readme is small, so we can pay the price of adding it.

On a related note, I am very pleased the Go team implemented the
`//go:embed` feature, so we can get rid of this bindata thing.
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 3, 2021
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 3, 2021
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 3, 2021
bassosimone added a commit to ooni/probe-engine that referenced this issue Feb 3, 2021
bassosimone added a commit to ooni/probe-cli that referenced this issue Feb 3, 2021
* doc: merge the engine and the cli readmes

Part of ooni/probe#1335

* refactor: we don't wanna export pkg/oonimkall/tasks

See ooni/probe#1335
@bassosimone
Copy link
Contributor Author

The merge is complete. We can now continue working on the engine inside of probe-cli. We have clearly deprecated the probe-engine repository. We will keep using it as a container for past issues. We will route future contributions to probe-cli/internal/engine.

ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
This is how I did it:

1. `git clone https://github.com/ooni/probe-engine internal/engine`

2. ```
(cd internal/engine && git describe --tags)
v0.23.0
```

3. `nvim go.mod` (merging `go.mod` with `internal/engine/go.mod`

4. `rm -rf internal/.git internal/engine/go.{mod,sum}`

5. `git add internal/engine`

6. `find . -type f -name \*.go -exec sed -i 's@/ooni/probe-engine@/ooni/probe-cli/v3/internal/engine@g' {} \;`

7. `go build ./...` (passes)

8. `go test -race ./...` (temporary failure on RiseupVPN)

9. `go mod tidy`

10. this commit message

Once this piece of work is done, we can build a new version of `ooniprobe` that
is using `internal/engine` directly. We need to do more work to ensure all the
other functionality in `probe-engine` (e.g. making mobile packages) are still WAI.

Part of ooni/probe#1335
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* fix: add missing files causing tests to fail

See ooni/probe#1335 (comment)

* fix: toggle verbose so we better understand the tests output
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* refactor: build miniooni from toplevel

Of course, also move the specific test checking whether we are
still able of building miniooni.

Part of ooni/probe#1335

* build for current branch just to confirm

* fix: correct the path where linux/arm binary is

* okay, it works, we can remove the special rule
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
This diff is part of ooni/probe#1335.

We are moving more probe-engine workflows to toplevel.

The general idea here is to migrate all possible workflows and to
delete the ones that we cannot use in this repo (if any).
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* refactor: start building an Android package

Part of ooni/probe#1335.

This seems also a good moment to move some packages out of the
engine, e.g., oonimkall. This package, for example, is a consumer
of the engine, so it makes sense it's not _inside_ it.

* fix: committed some stuff I didn't need to commit

* fix: oonimkall needs to be public to build

The side effect is that we will probably need to bump the major
version number every time we change one of these APIs.

(We can also of course choose to violate the basic guidelines of Go
software, but I believe this is bad form.)

I have no problem in bumping the major quite frequently and in
any case this monorepo solution is convinving me more than continuing
to keep a split between engine and cli. The need to embed assets to
make the probe more reliable trumps the negative effects of having to
~frequently bump major because we expose a public API.

* fix: let's not forget about libooniffi

Honestly, I don't know what to do with this library. I added it
to provide a drop in replacement for MK but I have no idea whether
it's used and useful. I would not feel comfortable exposing it,
unlike oonimkall, since we're not using it.

It may be that the right thing to do here is just to delete the
package and reduce the amount of code we're maintaining?

* woops, we're still missing the publish android script

* fix(publish-android.bash): add proper API key

* ouch fix another place where the name changed
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* refactor: miniooni should be outside of the engine

This is part of ooni/probe#1335. We also need
to think whether we wanna keep libminiooni and miniooni separated.

The previous use case for having a top-level libminiooni was that of
enabling others to integrate miniooni into other binaries.

This was usegul when studying internet censorship in Spain in May 2020.

I am wondering whether we should be keeping this complexity. I am not
sure about this and probably we should be killing it.

(In any case, reducing complexity is not the objective of this diff,
since I would like instead to move things around with minimal changes
and make sure we have a ~good repository organization here.)

* fix: import in libminiooni
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* refactor: move more commands to internal/cmd

Part of ooni/probe#1335.

We would like all commands to be at the same level of engine
rather than inside engine (now that we can do it).

* fix: update .gitignore

* refactor: also move jafar outside engine

* We should be good now?
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* refactor: enable QA tests and jafar self test

Part of ooni/probe#1335

* chore: make sure all workflows run on release branches
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* refactor: enable automatic iOS builds

Part of ooni/probe#1335

* fix: go mod tidy
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* chore: remove duplicate code of conduct

* chore: remove AUTHORS file

I doubt this actually has any value in the era of GitHub.

* chore: move CODEOWNERS to toplevel

* chore: move CONTRIBUTING.md to toplevel and adapt it

* chore: remove duplicated LICENSE file

* chore(engine): remove now-obsolete design document

* chore: remove the testusing test

We're not going to make this code importable from third parties
like we did for probe-engine. It seems this feature was only used
for the experiment in Spain so it makes sense to drop it.

* chore: enable code generation tests

See ooni/probe#1335

* chore: enable code-ql checks

* cleanup: remove libooniffi code and tests

It seems this code is not used. We are not aware of anyone using it. And we
don't want to expose it publicly as an API. So, what to do?

I guess it's fine to delete it. If there is anyone that needs it, we have
in the history a reference to it and we can always reinstate it.

* chore: move issue templates to ooni/probe
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
Part of ooni/probe#1335.

Motivation: we want all workflows to be green only when we are
approaching a release. It's fine if some less core tests are
failing during the development process. We have daily builds anyway
so we know of new breakages the day after, which is OK.
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* doc: ensure all top dirs have an explanatory README

This makes the repository a lil bit nicer to newcomers.

Part of ooni/probe#1335

* fix: re-run bindata to embed the README

The readme is small, so we can pay the price of adding it.

On a related note, I am very pleased the Go team implemented the
`//go:embed` feature, so we can get rid of this bindata thing.
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
ainghazal pushed a commit to ainghazal/probe-cli that referenced this issue Mar 8, 2022
* doc: merge the engine and the cli readmes

Part of ooni/probe#1335

* refactor: we don't wanna export pkg/oonimkall/tasks

See ooni/probe#1335
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
This diff includes some cleanups while we finish moving packages
away from the internal/engine package.

With this diff, we have finally finished merging probe-engine into
probe-cli, a process initiated in February 2021.

See ooni/probe#1335.

Closes ooni/probe#2115.
bassosimone added a commit to ooni/probe-cli that referenced this issue Jan 16, 2023
This diff includes some cleanups while we finish moving packages
away from the internal/engine package.

With this diff, we have finally finished merging probe-engine into
probe-cli, a process initiated in February 2021.

See ooni/probe#1335.

Closes ooni/probe#2115.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant