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

[Tools] Ubuntu production environment "bootstrap" script #3981

Closed
wants to merge 10 commits into from
Closed

[Tools] Ubuntu production environment "bootstrap" script #3981

wants to merge 10 commits into from

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Oct 3, 2018

This pull request has been heavily refactored so a lot of the discussion may not make sense.

Brief summary of changes

bootstrap verifies loris dependencies such as Apache/PHP versions and misc system tools and optionally installs them. This is meant to be run only on development environments as production environments will in the future have all of this handled by a packaged release.

PHP_CLI_Helper consists of helper functions to make it easier to write PHP scripts for the command line.

Further work

  • Make something equivalent for CentOS as it's another environment that our team uses for development.

To test this change...

  • tools/bootstrap and let me know if there are any errors!

@johnsaigle johnsaigle added Feature PR or issue introducing/requiring at least one new feature Cleanup PR or issue introducing/requiring at least one clean-up operation Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix Blocked PR awaiting the merge, resolution or rejection of an other Pull Request labels Oct 3, 2018
@johnsaigle johnsaigle changed the title Move toward scripts to rule them all [Tools] Use Scripts To Rule Them All paradigm Oct 3, 2018
@johnsaigle johnsaigle changed the title [Tools] Use Scripts To Rule Them All paradigm [Tools] Use "Scripts To Rule Them All" paradigm Oct 3, 2018
@johnsaigle johnsaigle added Blocked PR awaiting the merge, resolution or rejection of an other Pull Request [branch] major and removed Blocked PR awaiting the merge, resolution or rejection of an other Pull Request labels Oct 3, 2018
@johnsaigle johnsaigle removed Blocked PR awaiting the merge, resolution or rejection of an other Pull Request Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels Oct 3, 2018
Makefile Outdated
npm run compile
git describe --tags --always > VERSION

dev:
Copy link
Contributor Author

@johnsaigle johnsaigle Oct 3, 2018

Choose a reason for hiding this comment

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

This functionality is preserved by setup and bootstrap; one can add a 'dev' argument upon invoking these scripts and have composer do the appropriate thing.

@johnsaigle johnsaigle removed the Cleanup PR or issue introducing/requiring at least one clean-up operation label Oct 3, 2018
@johnsaigle johnsaigle changed the title [Tools] Use "Scripts To Rule Them All" paradigm [Tools] Streamline common tasks with "Scripts To Rule Them All" paradigm Oct 3, 2018
script/bootstrap Outdated Show resolved Hide resolved
script/bootstrap Outdated Show resolved Hide resolved
script/bootstrap Outdated
/**
* Prints a list of missing apt packages and prompts user to install them.
*
* @param array $requirements Required packages determined missing earlier
Copy link
Contributor

Choose a reason for hiding this comment

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

since other lines end with a dot, this line should also end with a dot
same of other doc block

script/bootstrap Outdated Show resolved Hide resolved
script/bootstrap Outdated Show resolved Hide resolved
@driusan
Copy link
Collaborator

driusan commented Oct 9, 2018

Replacing a simple < 10 line Makefile with hundreds of lines of bespoke PHP scripts in order to adopt a third party's internal procedures is not a step forward.

If you want to simplify running the setup add some standard targets like make clean, make test or make install.

@driusan driusan closed this Oct 9, 2018
@ridz1208
Copy link
Collaborator

hmmm, let's see:

  • first, the <10 line makefile is doing soooo much less than the hundreds of lines of bespoke php
  • make is a tool not a standard and we are in dying need of standards (especially some not defined by LORIS for LORIS) and these ones have some documentation and play ball with github... especially that they are described by GITHUB Eng and we use github for PRs, issues, projects, why not standards !!
  • Talking about standards, the intent of scripts to rule them all is to standardize these scripts in multi-project development and seems to me that LORIS, LORIS-MRI, CCNA, IBIS, BIOBANK, etc... do fit that picture pretty nicely. imagine having a standard way of setting them up or cheking for dependencies istead of ad hoc-ing our way during the installation.
  • let's also not omit the fact that we can write these scripts in whatever language we want and we can have them do everything we need. no need to code if and for loops in bash !!! no we can use python, php what ever... that sounds pretty nice with an actual potential of being maintained
  • and how about just this... someone actually made the effort to code this and test it instead of just talking about it and dreaming.

the way I see it, if LORIS is WAY TOO ATTACHED to makefiles, then just create makefiles and have them called FROM the scripts to rule them all, i dont care as long as I dont have to explain to each and every new developer how to run a test, how to run phpcs, how to run phan, ...

a quick read https://www.jefframnani.com/project-build-protocol/

@ridz1208 ridz1208 reopened this Oct 11, 2018
@ridz1208 ridz1208 added the Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties label Oct 11, 2018
@driusan
Copy link
Collaborator

driusan commented Oct 11, 2018

I'm just going to copy and paste the blog post that you linked to here which applies to this repo:

Scenario One: One Project, Many Commands

In this scenario, you’re mostly working on one main app all day. Your company’s monolith, for example.

Let’s say you’re working in a Django project. To run the Django unit tests you run:

DJANGO_SETTINGS_MODULE=myapp.settings.local_test python manage.py test

To run the JavaScript front-end tests you run:

node_modules/.bin/gulp test

Now, I’m lazy. I don’t want to have to remember these two commands, their environment variables, or their arguments. What I normally do in this situation is write a simple Makefile.

test: test-py test-js

test-py:
    DJANGO_SETTINGS_MODULE=myapp.settings.local_test REUSE_DB=1 ./manage.py test

test-js:
    node_modules/.bin/gulp test

Now when I come back to this project, I only have to remember to run make
test, and all my tests get run.

(In my real Makefile I use .PHONY targets to tell Make that these names don’t correspond to actual files.)

For this example, I only created a test target, but the common targets I like to use are: build, test, run, and dist. These targets create a common protocol I can depend on. They also serve as a form of executable documentation for how to do one of these tasks.

Makefiles can get pretty hairy, pretty quickly, so the minute a task starts getting hard to read or maintain, I’ll write a separate script to perform the task, then just delegate to the external script within the Makefile.

Why Make? Because it’s ubiquitous and it’s easy to install.

At 9 lines which don't branch or loop, you can't even claim we're at the "pretty hair, pretty quickly" point that would necessitate delegating to an external script inside of the Makefile. The complexity in this PR is added by removing the Makefile and attempting to rewrite the functionality in another language that isn't well suited to the task of building software.

The other scenerio in the blog post (which is a little tortured to say applies to LORIS, although arguably does with Loris-MRI) doesn't provide any rational for abandoning that approach rather than just standardizing the makefile targets across the projects. Here are a list of standard Makefile targets used by GNU: https://www.gnu.org/prep/standards/html_node/Standard-Targets.html (not all apply to LORIS. I'm also surprised that "test" isn't included.) Adopting a real standard that's been standardized for building software for decades rather than a blog post standard would have a number of benefits such as making it trivial to build packages for any supported OS using their standard procedures.

@ridz1208
Copy link
Collaborator

@driusan

Call me cynic but I think

Makefiles can get pretty hairy, pretty quickly, so the minute a task starts getting hard to read or maintain, I’ll write a separate script to perform the task, then just delegate to the external script within the Makefile.

applies to us rather well.

I think that even though it is using another language that isn't well suited to the task of building software it might have more chances to be maintained by your average LORIS developer who probably hasn't touched a make file in the last 10 years.

At 9 lines which don't branch or loop, you can't even claim we're at the "pretty hair, pretty quickly" well that's because the 9 lines are not doing much. I'd be curious to see what the makefiles would look like with the same functionality as the scripts to rule them all (checking for versions, maybe handling errors....)

The other sceario in the blog post (which is a little tortured to say applies to LORIS, although arguably does with Loris-MRI) in our effort to streamline project releases, I'm surprised you don't see the potential of using this for projects, I would loove to havea single lioner for every release scripts/update. And wait... I thought the plan was to extract all modules into individual repos... that sounds pretty hairy to me.

As far as the standards are concerned, you wanna choose the make standards over the scripts to rule them all, by all means do, but no one has taken initiative to do that and I don't see anyone doing it... (test is included in the make check btw) like I said earlier we gonna appreciate the fact that the work has been done !!! I'm not saying to merge this blindly but it's at least worth a discussion and some testing.

I would like to see a comparison between the GNU way that's been used for "decades" vs the github way that might be used in the next decades ?? who knows maybe we'd actually be on to something futuristic there

@johnsaigle
Copy link
Contributor Author

Just to be clear this PR wasn't written for the purpose of replacing the Makefile. I did the development toward creating these scripts and realized that they covered what the Makefile did (and a bunch more, obviously). I'm not opposed to having both live side-by-side. It's no trouble to add the file back and do something like below if we want Makefiles.

test:
    script/test

I do believe that the bootstrap and test logic is pretty complex/"hairy" to do in Bash. Without getting in to language features, more developers on our team are skilled with PHP over Bash and so I wrote these so as to be more easily maintainable for a greater number of people. (Same as the update script).

@johnsaigle johnsaigle added Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix and removed Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties labels Dec 19, 2018
@johnsaigle
Copy link
Contributor Author

TODO:

  • Revert changes to Makefile and travis.yml
  • Rename instances of scripts to tools
  • Document changes, etc. "tools/bootstrap.php can be used to verify or install missing system requirements for Ubuntu. Used only for dev environments...."
  • Delete setup in favour of make clean target with the same bash commands
  • Add a make test target that runs tools/test.php. This script may need to be scrapped; will coordinate with @driusan and PR Add make check target #4002

@driusan driusan added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Dec 20, 2018
@johnsaigle johnsaigle changed the title [Tools] Streamline common tasks with "Scripts To Rule Them All" paradigm [Tools] Production enviroment " Jan 3, 2019
@johnsaigle johnsaigle changed the title [Tools] Production enviroment " [Tools] Ubuntu production environment "bootstrap" script Jan 3, 2019
@johnsaigle johnsaigle removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Jan 3, 2019
@johnsaigle johnsaigle dismissed PapillonMcGill’s stale review January 3, 2019 18:15

Pretty sure this is resolved

@johnsaigle johnsaigle removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Jan 3, 2019
Copy link
Contributor

@PapillonMcGill PapillonMcGill left a comment

Choose a reason for hiding this comment

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

almost there!

tools/PHP_CLI_Helper.class.inc Show resolved Hide resolved
tools/PHP_CLI_Helper.class.inc Outdated Show resolved Hide resolved
tools/PHP_CLI_Helper.class.inc Outdated Show resolved Hide resolved
tools/PHP_CLI_Helper.class.inc Show resolved Hide resolved
PapillonMcGill
PapillonMcGill previously approved these changes Jan 8, 2019
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

Other than all the references to "bash", this isn't conforming to the discussion we had last year about how this should be approached. This is still trying to install everything, when what we need is a script which verifies dependencies and reports the missing/incorrect versions of things that are installed before any attempt is made to install anything.

The PR itself should also be closed and re-sent when it's ready so that the discussion doesn't get confused with previous discussions from before major refactoring.

<?php

/**
* Use bash to determine if certain unix tools are installed. Which is used for
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't using bash, it's using the shell

{
// `which` returns empty if a tool is not installed.
// shell_exec captures this output.
if (shell_exec("which $tool")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would be more reliable if it used exec and checked the shell return code, rather than trying to parse stdout.

if (shell_exec("which $tool")) {
return true;
}
// check installed pacakages with dpkg. Returns 0 if installed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure about that? dpkg(1) says:

EXIT STATUS         top

       0      The requested action was successfully performed.  Or a check
              or assertion command returned true.

       1      A check or assertion command returned false.

       2      Fatal or unrecoverable error due to invalid command-line
              usage, or interactions with the system, such as accesses to
              the database, memory allocations, etc.

(but doesn't really clarify what "a check or assertion command" is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm well I mean the -s flag gives the status of a package and returns 1 when not found so I think it works in this case.

tools/PHP_CLI_Helper.class.inc Outdated Show resolved Hide resolved
tools/PHP_CLI_Helper.class.inc Outdated Show resolved Hide resolved
tools/PHP_CLI_Helper.class.inc Outdated Show resolved Hide resolved
tools/PHP_CLI_Helper.class.inc Outdated Show resolved Hide resolved
tools/PHP_CLI_Helper.class.inc Outdated Show resolved Hide resolved
"ERROR: LORIS requires Apache v$required_apache or higher."
. PHP_EOL
. "This must be done manually as it has possible security ramifications."
. PHP_EOL
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second sentence is unnecessary.

@@ -0,0 +1,214 @@
#!/usr/bin/env php
# This script verifies a development installation of LORIS by ensuring that
# the system has all of the required dependencies such as the correct PHP and
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if PHP isn't installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a check for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the script is written in PHP, so how can it check that?

driusan and others added 3 commits February 25, 2019 12:51
Co-Authored-By: johnsaigle <4022790+johnsaigle@users.noreply.github.com>
add dave's suggestion

Co-Authored-By: johnsaigle <4022790+johnsaigle@users.noreply.github.com>
Co-Authored-By: johnsaigle <4022790+johnsaigle@users.noreply.github.com>
@johnsaigle
Copy link
Contributor Author

Closing this PR and will re-open a fresh one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PR or issue introducing/requiring at least one new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants