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

adding distillery as the release manager and ensuring the release starts #543

Merged
merged 14 commits into from
Nov 1, 2018

Conversation

InoMurko
Copy link
Member

  • I've added Distillery dependency that we can use as the OTP release process.
  • I've removed all Mix.env() runtime calls, because Mix isn't available in a OTP release, making the release unable to start.

@InoMurko InoMurko force-pushed the release branch 2 times, most recently from 44d3773 to 752dfd9 Compare October 27, 2018 15:22
Copy link
Contributor

@hayesgm hayesgm left a comment

Choose a reason for hiding this comment

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

A few questions on environments and releases. Also, can you do a refresh rebase off of master?

rel/config.exs Outdated
environment :prod do
set include_erts: true
set include_src: false
set cookie: :"%POy.W>cr1@.Qe217jz;d.m0Uhx]$h8Ae$3.i*,U;jH@]U^T}&sLkg12;,5dc)P5"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we load cookie from somewhere else or just not include it at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

At this moment I think the purpose of having the release built in CI is that we're ensuring it doesn't get broken. Any tarball release that might deploy the tarball would want to overwrite the cookie anyway.

Setting it to something that doesn't look generated might make more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the longer run, having acceptance tests that would run against a release would prove the release working as well.

apps/ex_wire/lib/ex_wire/config.ex Show resolved Hide resolved
at: .

- run: mix release

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add the release tar.gz as an artifact so it's stored by CircleCI

Copy link
Member Author

Choose a reason for hiding this comment

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

It would make sense to pack the artefacts + logs once we have acceptance test run against it.

@InoMurko InoMurko force-pushed the release branch 8 times, most recently from c224435 to 2c49708 Compare October 28, 2018 16:35
@InoMurko
Copy link
Member Author

Sorting issues with RocksDB...

Copy link
Contributor

@hayesgm hayesgm left a comment

Choose a reason for hiding this comment

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

Alright, looks good to me. Thanks!

@hayesgm hayesgm mentioned this pull request Oct 28, 2018
@InoMurko InoMurko force-pushed the release branch 8 times, most recently from 850c262 to 05c4adc Compare October 29, 2018 18:00
@hayesgm
Copy link
Contributor

hayesgm commented Oct 30, 2018

@InoMurko I was taking a look at this last night-- it's definitely having a problem compiling the rox dependency, breaking during a rust build of a C++ artifact? But, the bigger question is actually: it works fine building test (MIX_ENV=test mix compile)-- is it not building that artifact in test or is the environment different?

@InoMurko
Copy link
Member Author

InoMurko commented Oct 30, 2018

Hey @hayesgm!
What happens is that rox wants to optimise the cargo build for a release. As per https://hexdocs.pm/rustler/basics.html and the mix.exs of the rox dependency:
defp rustler_crates do [ rox_nif: [path: "native/rox_nif", cargo: :system, default_features: false, features: [], mode: :release], ] end

What I think it happens is that the compilation eats the all available memory in the container executing the job. I've been aligning with @atoulme to increase the CI plan that would allow us to increase the memory size - it's only 4gb at the moment. That should solve it. I've tried compiling a release on a bigger linux instance and it worked.

@hayesgm
Copy link
Contributor

hayesgm commented Oct 30, 2018

I guess I'm a little disheartened that rust takes 4GB of memory to compile? I could imagine it taking longer, but that much memory to build a KV database?

@InoMurko InoMurko force-pushed the release branch 2 times, most recently from 523d2d7 to 01acb28 Compare October 30, 2018 18:42
@InoMurko
Copy link
Member Author

That's my current take (with limited time at devcon and the output being internal compiler error: Killed with exit code 4). Tried also rustc nightly build and it comes up the same.

@InoMurko
Copy link
Member Author

InoMurko commented Oct 31, 2018

Managed to get this working by swapping the RocksDB dependency used with Erlang's impl https://gitlab.com/barrel-db/erlang-rocksdb .

That pointed me to the fact that db handles are not being closed. Probably because the previous :rox dependency didn't support that. This awesome work from Benoit does, we should implement that in a later stage.

Please review again @hayesgm @masonforest @atoulme and others.

@hayesgm
Copy link
Contributor

hayesgm commented Oct 31, 2018

Great work getting this building. Do you know if it's possible to build for multiple architectures? E.g. one for linux, one for macOS and one for Windows?

@InoMurko
Copy link
Member Author

I’m not 100% for windows. Everything else should work.

@hayesgm
Copy link
Contributor

hayesgm commented Oct 31, 2018

On MacOS:

$ ./bin/mana
./erts-10.0.5/bin/erl: line 13: ./erts-10.0.5/bin/erlexec: cannot execute binary file
./erts-10.0.5/bin/erl: line 13: ./erts-10.0.5/bin/erlexec: Undefined error: 0
Unusable Erlang runtime system! This is likely due to being compiled for another system than the host is running

@InoMurko
Copy link
Member Author

Oh I wasn’t sure what you meant. You took the release artifact from Circle? You would have to build it separately. Afaik, circle allows docker, mac but not windows.

@hayesgm
Copy link
Contributor

hayesgm commented Oct 31, 2018

Yeah, it'd be cool if we could build for macOS, for instance, in a docker container. But for now, this is great and we should merge to master!

Copy link
Contributor

@hayesgm hayesgm left a comment

Choose a reason for hiding this comment

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

looks great, thanks for getting this working.

.credo.exs Outdated Show resolved Hide resolved
Copy link
Contributor

@MattMSumner MattMSumner left a comment

Choose a reason for hiding this comment

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

Once the credo change is reverted, this LGTM.

@InoMurko InoMurko force-pushed the release branch 3 times, most recently from 991683a to a1c594e Compare November 1, 2018 15:32
@InoMurko InoMurko merged commit de3454b into mana-ethereum:master Nov 1, 2018
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.

4 participants