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

[Instrument Manager] Proof of concept load instrument manager from external repository #3921

Closed
wants to merge 16 commits into from

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Sep 4, 2018

This updates the instrument manager to be loaded from a separate git
repo and composer required. Theoretically, this allows us to more
rapidly iterate on it, and make releases of the instrument manager
separately from LORIS releases to get module bug fixes / features out
faster by running "composer update".

The instrument manager is used as a test case for this because it is
relatively self contained, lightly used, and doesn't have any ajax
that needs to be converted to a router.

LORIS (including the instrument manager) should work identically
before and after this change, since it adds the instrument manager
as a dependency to LORIS.

To test this PR

test the instrument manager. Note that you may need to change the "vcs"
line to a local repository and tag a v21.0.0 to test this PR, because there's
a bootstrapping problem where the module requires >20, which isn't
released yet. (We should also figure out a way to eliminate the circular
dependency.)

I've used created/used the loris2 github organization as a test
for this (loris was already taken), this is not necessarily its
final home (but making a aces repo for every single module is
probably not feasible.)

…ternal repository

This updates the instrument manager to be loaded from a separate git
repo and composer required. Theoretically, this allows us to more
rapidly iterate on it, and make releases of the instrument manager
separately from LORIS releases to get module bug fixes / features out
faster by running "composer update".

The instrument manager is used as a test case for this because it is
relatively self contained, lightly used, and doesn't have any ajax
that needs to be converted to a router.

LORIS (including the instrument manager) should work identically
before and after this change. To test this PR, test the instrument
manager. Note that you may need to change the "vcs" line to a local
repository and tag a v21.0.0 to test this PR, because there's a
bootstrapping problem where the module requires >20, which isn't
released yet.

I've used created/used the loris2 github organization as a test
for this (loris was already taken), this is not necessarily its
final home (but making a aces repo for every single module is
probably not feasible.)
"autoload" : {
"psr-4": {
"LORIS\\": "src/"
},
"classmap": ["project/libraries", "php/libraries", "php/exceptions"]
"classmap": ["php/libraries", "php/exceptions"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The "project" hack had to be removed here or composer update dies with the error:

                                                                               
  [RuntimeException]                                                           
  Could not scan for classes inside "/Users/driusan/Code/Loris-InstrumentMana  
  ger/vendor/aces/loris/project/libraries" which does not appear to be a file  
   nor a folder                                                                
                                                                               

update [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--lock] [--no-custom-installers] [--no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [--with-dependencies] [--with-all-dependencies] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--prefer-stable] [--prefer-lowest] [-i|--interactive] [--root-reqs] [--] [<packages>]...

@@ -10,7 +13,8 @@
"psr/http-message": "~1.0",
"psr/http-server-handler" : "*",
"psr/http-server-middleware" : "*",
"zendframework/zend-diactoros": "^1.6"
"zendframework/zend-diactoros": "^1.6",
"loris/instrumentmanager" : "dev-master"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be loris2 as per your PR description? Could fix the Travis error also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding is that it needs to match what's in the composer.json of the repo, and it's just "loris" there: https://github.com/loris2/instrumentmanager/blob/master/composer.json#L2

Copy link
Contributor

@johnsaigle johnsaigle Sep 4, 2018

Choose a reason for hiding this comment

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

Hmmm maybe Travis si having an issue with this line then?

https://github.com/loris2/instrumentmanager/blob/f0ce955e2e2e3dccf57937ecefd91b5ff17b8d99/composer.json#L6

I'm not clear on what vcs is meant to do but it seems like Travis would have problems getting info from your laptop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

..though I just noticed the URL for "loris" is wrong in the other repo's composer.json, so I'll try fixing that and see if it helps

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The "vcs" line tells composer that it should look for packages in that repo, otherwise composer will just use packagist.org. (If this works we should probably make packagist releases of LORIS and its modules, but until we're committed to adding modules with "composer require" it's probably not worth it.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was that line that was the problem.. now the error is more like what I expected since there's no release with this change:

composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Your requirements could not be resolved to an installable set of packages.
  Problem 1
    - Installation request for loris/instrumentmanager dev-master -> satisfiable by loris/instrumentmanager[dev-master].
    - loris/instrumentmanager dev-master requires aces/loris >20.0 -> no matching package found.

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 not sure how I can bootstrap this before it's merged.. I can't have the instrumentmanager require the major branch either, because it's also not merged into major.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it rely on the code changes below and that's why it needs 21? Is it possible those changes could go into 20.x?

Or maybe this comes out in 21.1 after the below is merged. Essentially merge the underlying code without including the test example.

I've got a hazy understanding of this but... don't see how you get out of this dependency circle without delaying some piece of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, these are the code changes that I had to do in order to get it to load when using a local file:// url for the VCS line.

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'll try eliminating the circular dependency by not declaring the loris dependency on the other side until there's a release which includes the changes

@driusan driusan added State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed [branch] major labels Sep 4, 2018
@johnsaigle
Copy link
Contributor

screenshot from 2018-09-10 11-55-35

@driusan Could you add more information to the PR description as to how to properly test this? I ran composer install after checking out your branch and though I got - Installing loris/instrumentmanager (dev-master 6030a1b) Cloning 6030a1bed8 from cache, I'm still not able to see the Instrument Manager.

I'm not clear on whether the local vcs is necessary now that you have the instrumentmanager repo.

@driusan
Copy link
Collaborator Author

driusan commented Sep 10, 2018

@johnsaigle It should be transparent with no changes required. What are you using for a web server? What's the URL in your web browser? The fact that the error message has ?lorispath= in it is a little disturbing.

@driusan
Copy link
Collaborator Author

driusan commented Sep 10, 2018

@johnsaigle can you check if you get the ?lorispath in other 404 errors on the major branch?

@driusan
Copy link
Collaborator Author

driusan commented Sep 10, 2018

@johnsaigle 406b658 should fix it, but maybe that part should go to bugfix..

@johnsaigle
Copy link
Contributor

@driusan

The original URL was https://jsaigle-dev.loris.ca/instrument_manager/

lorisPath does not appear for other 404 errors on this branch.

screenshot from 2018-09-10 12-45-22

Server Info

apache2 -v
Server version: Apache/2.4.34 (Ubuntu)
Server built:   2018-07-26T02:36:08
uname -a
Linux jsaigle-dev 4.15.0-33-generic #36-Ubuntu SMP Wed Aug 15 16:00:05 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

@johnsaigle
Copy link
Contributor

@driusan I'm getting the same error after the most recent commit.

lorisadmin at jsaigle-dev in /var/www/loris on (HEAD detached at dave/ExternalInstrumentManager)*
12:47:43 $ git log | head -10
commit 406b6588228dce0437cc89071510a89f1ec266e7
Author: Dave MacFarlane <driusan@gmail.com>
Date:   Mon Sep 10 12:44:02 2018 -0400

    Convert mod_rewrite rewrites back to the original request

@zaliqarosli zaliqarosli added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Nov 19, 2018
@johnsaigle
Copy link
Contributor

Note that since #4142 has been merged that this PR may be affected,

@johnsaigle johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Nov 28, 2018
@johnsaigle
Copy link
Contributor

@driusan If you rebase this onto major I will be able to help debug/test now that #4163 is merged. That's what was causing my problems above.

Instrument manager has changed a lot since #4142 is merged but we can still test this install as a PoC.

@johnsaigle johnsaigle removed the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Dec 19, 2018
@driusan
Copy link
Collaborator Author

driusan commented Dec 20, 2018

@johnsaigle as a proof of concept getting this merged is a low priority right now, as long as we know that it's possible to develop modules this way in the future.

@samirdas
Copy link
Contributor

Can you decide if this should even stay open. @driusan

@driusan
Copy link
Collaborator Author

driusan commented Jan 28, 2019

Will close this for now, look into it again later.

(We should at least incorporate the Module/Smarty_hook bugfixes even without the instrument manager, but it doesn't need to be done as part of this PR.)

@driusan driusan closed this Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs work PR awaiting additional work by the author to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants