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

Arweave Should Not Build Before Being Stopped #621

Conversation

humaite
Copy link
Collaborator

@humaite humaite commented Sep 23, 2024

This commit fixes an issue related to ./bin/stop script and ./bin/arweave.env. When stopping Arweave application in dev environment, a full release is always created. It is not clean and could probably leads to errors (even if was not the case previously).

The compilation is now made only when SCRIPT_ACTION variable is not set to "stop" or if ./bin/arweave-dev does not exist.

bin/arweave.env Outdated
# is not set to stop. A full recompilation is not needed when the
# application was already started. It is required if arweave-dev
# does not exist though.
if [ "${SCRIPT_ACTION}" != "stop" ] || ! [ -f "${SCRIPT_DIR}/arweave-dev" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than SCRIPT_ACTION... maybe some other more generic arg like "SKIP_BUILD" or something? That way we only have to set the parameter in scripts that need it. There are a lot of scripts that source arweave.env and currently don't set SCRIPT_ACTION

also if you can find a better way to do the arweave.env initialization. I don't love the current approach of setting environment variables, and since it's called via source we can't esily pass in command line arguments (which is why we have the kludgy "set an env var and then check it inside arweave.env")

The key thing is making sure all scripts which run arweave (e.g. arweave-server, start, stop, benchmark-xxx, data-doctor, shell, test, et....) all are launched by default under the same environment

We had some hairy bugs before where the two scripts (e.g. start vs. arweave-server) were launched with different flags which caused some hard to debug performance issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rather than SCRIPT_ACTION... maybe some other more generic arg like "SKIP_BUILD" or something? That way we only have to set the parameter in scripts that need it. There are a lot of scripts that source arweave.env and currently don't set SCRIPT_ACTION

Correct. I will rename it SKIP_BUILD.

also if you can find a better way to do the arweave.env initialization. I don't love the current approach of setting environment variables, and since it's called via source we can't esily pass in command line arguments (which is why we have the kludgy "set an env var and then check it inside arweave.env")

What do you really want? Something like the following code?

./bin/arweave start
./bin/arweave stop
./bin/arweave console
./bin/arweave test

The best way would be to have only one entry-point and not many, a bit like bitcoin or ethereum do but it will be kinda complex right now. Do you have the list of the most used commands, like, we can start with a script doing the commands used 80% of the time, and then improve it when needed.

The key thing is making sure all scripts which run arweave (e.g. arweave-server, start, stop, benchmark-xxx, data-doctor, shell, test, et....) all are launched by default under the same environment

Does arweave really need environment variable? It seems the only environment needed by arweave is TERM (see apps/arweave/src/ar.erl). So yeah, creating an unique entry-point controlled by flags can be a first step.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added SKIP_BUILD variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah a single entry point might be better, but also agree not for this PR. Main issue with a single entry point is just the one extra layer of nested flags. E.g. the benchmark-xxx and data-doctor commands already have the definition of a subcommand and then flags specific to that subcommand. So we'd essentially need to have command, sub-commands, and sub-sub-commands which get hairy.

Probably best to defer that change until we can revisit how we luanch all this stuff in general (e.g. if we're moving towards a form of UI, maybe the commandline complexity becomes less of an issue) So for now, no change. We just have to make sure each of the multiple endpoints are launched in a consistent state (as they are now)

@humaite humaite force-pushed the imporovement/614-dependencies-are-sometime-compiled-before-stopping-a-node branch from fcb97d9 to eab488d Compare October 11, 2024 09:02
bin/stop Outdated

# Sets $ARWEAVE and $ARWEAVE_* variables
export SKIP_BUILD=yes
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need the export. I'm always fuzzy on that, but if you export skip_build I think it might persist in the shell environment after you run the script? If so, then we don't want that.

e.g. SCRIPT_DIR and SCRIPT_ACTION aren't exported, but are still picked up by the source arweave.env line which is all we need for now.

@humaite humaite force-pushed the imporovement/614-dependencies-are-sometime-compiled-before-stopping-a-node branch from eab488d to a16dd82 Compare October 11, 2024 15:46
This commit fixes an issue related to `./bin/stop` script and
`./bin/arweave.env`. When stopping Arweave application in
dev environment, a full release is always created. It is not
clean and could probably leads to errors (even if was not the
case previously).

If `SKIP_BUILD` variable is defined then arweave is not
compiled.
@humaite humaite force-pushed the imporovement/614-dependencies-are-sometime-compiled-before-stopping-a-node branch from a16dd82 to dd69558 Compare October 11, 2024 15:49
@JamesPiechota JamesPiechota merged commit 12073d3 into ArweaveTeam:master Oct 14, 2024
42 of 65 checks passed
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.

2 participants