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

orientdb 2.2.2 #2004

Closed
wants to merge 9 commits into from
Closed

Conversation

austinsmorris
Copy link
Contributor

@austinsmorris austinsmorris commented Jun 15, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --strict --online <formula> (after doing brew install <formula>)?

@austinsmorris
Copy link
Contributor Author

This PR makes #1589 obsolete. That PR should be closed (if this passes)...

@austinsmorris austinsmorris mentioned this pull request Jun 15, 2016
@austinsmorris
Copy link
Contributor Author

@MikeMcQuaid @UniqMartin, I could use a little help here. I was able to work past @arxpoetica's issues with the pid and database locations in #1589. Now brew test --sandbox --verbose orientdb is passing for me locally, but this build it still failing. Any ideas?

@UniqMartin UniqMartin changed the title Update orientdb to version 2.2.2 orientdb 2.2.2 Jun 15, 2016
url "https://orientdb.com/download.php?email=unknown@unknown.com&file=orientdb-community-2.1.16.tar.gz&os=mac"
version "2.1.16"
sha256 "41ad0db53c418459d0efbf6a7f7e2b39f48467f1ec582efa925ceb38de3f3cc6"
url "http://orientdb.com/download.php?email=unknown@unknown.com&file=orientdb-community-2.2.2.tar.gz&os=mac"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't switch back to insecure HTTP if HTTPS works. It also looks like the URL can be simplified (according to the information in the OrientDB documentation):

url "https://orientdb.com/download.php?file=orientdb-community-2.2.2.tar.gz"

@austinsmorris
Copy link
Contributor Author

@UniqMartin, url has been updated, but I still have a failing build even though tests pass locally....

@UniqMartin
Copy link
Contributor

url has been updated, but I still have a failing build even though tests pass locally....

Yes, I noticed. I suspect it's an issue on our end, but I haven't had the time yet to investigate. (I hope to be able to do this shortly.) Meanwhile, the URL change caused an audit error. Please try to keep in mind to re-run brew audit --strict --online <formula> locally before pushing.

@austinsmorris
Copy link
Contributor Author

@UniqMartin, fixed the audit, thanks.

@UniqMartin
Copy link
Contributor

Alright, it's not a problem with our CI. I tried it locally and the formula still has several issues that need to be resolved for this to work.

  1. bin/shutdown.sh needs to be adjusted (just like bin/server.sh). Once I had installed and started the server with orientdb start, I was unable to stop it with orientdb stop. (I had to kill it manually using Activity Monitor.)
  2. The test tries to write to /usr/local/var and this is not allowed during the test. All directories where OrientDB needs to write to must be temporarily redirected to subdirectories of testpath (the working directory in test do). To verify this locally, stop all OrientDB processes and then delete (or move aside) everything that has been created by OrientDB on its first start, particularly /usr/local/var/db/orientdb/OSystem. If you then run brew test --sandbox --verbose orientdb, the test will fail:
    • ~/Library/Logs/Homebrew/orientdb/sandbox.test.log will have entries like:

      Jun 16 16:03:36 sandboxd[136]: java(90130) deny file-write-create /usr/local/var/db/orientdb/OSystem
      Jun 16 16:03:36 sandboxd[136]: java(90130) deny file-write-create /usr/local/var/db/orientdb/OSystem
      
    • /usr/local/var/log/orientdb/orientdb.err will have entries like:

      2016-06-16 16:03:36:728 INFO  {db=OSystem} Creating the system database 'OSystem' for current server [OSystemDatabase]
      Exception in thread "main" com.orientechnologies.orient.core.exception.ODatabaseException: Cannot create database 'OSystem'
              DB name="OSystem"
              at com.orientechnologies.orient.core.db.document.ODatabaseDocumentTx.create(ODatabaseDocumentTx.java:450)
              at com.orientechnologies.orient.server.OSystemDatabase.init(OSystemDatabase.java:160)
              at com.orientechnologies.orient.server.OSystemDatabase.<init>(OSystemDatabase.java:44)
              at com.orientechnologies.orient.server.OServer.initSystemDatabase(OServer.java:1222)
              at com.orientechnologies.orient.server.OServer.activate(OServer.java:342)
              at com.orientechnologies.orient.server.OServerMain.main(OServerMain.java:41)
      Caused by: com.orientechnologies.orient.core.exception.OStorageException: Cannot create folders in storage with path /usr/local/var/db/orientdb/OSystem
              DB name="OSystem"
              at com.orientechnologies.orient.core.storage.impl.local.paginated.OLocalPaginatedStorage.create(OLocalPaginatedStorage.java:118)
              at com.orientechnologies.orient.core.db.document.ODatabaseDocumentTx.create(ODatabaseDocumentTx.java:402)
              ... 5 more
      

That's about as far as I got. Maybe there are more problems and more stuff needs to be adjusted.

@UniqMartin UniqMartin added the needs response Needs a response from the issue/PR author label Jun 16, 2016
@austinsmorris
Copy link
Contributor Author

Great, thanks for the input! I'll keep working on it.

@ghost ghost removed the needs response Needs a response from the issue/PR author label Jun 16, 2016
mkpath "#{libexec}/log"
touch "#{libexec}/log/orientdb.err"
touch "#{libexec}/log/orientdb.log"
mkpath "#{var}/db/orientdb"
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably make this (and the one below)

(var/"db/orientdb").mkpath

@austinsmorris
Copy link
Contributor Author

@ilovezfs, thanks for the suggestions, I've made those changes. Do you know how I can stop orientdb from writing to /usr/local/var/db/orientdb during test? This causes the tests to fail.

@MikeMcQuaid
Copy link
Member

@austinsmorris That'd be orientdb specific but presumbly an environment variable or configuration file.

@UniqMartin
Copy link
Contributor

UniqMartin commented Jun 17, 2016

Do you know how I can stop orientdb from writing to /usr/local/var/db/orientdb during test? This causes the tests to fail.

Ideally, you would want to make sure that it also has its own PID file and log files as its undesirable that the test modifies these if the test every happens to be run by a user (contrary to our CI that always starts from a clean state). For the parts where you modify the shell scripts, something like that can be achieved by allowing those values to be overridden by environment variables and falling back to the defaults if unset, e.g.:

inreplace "#{libexec}/bin/server.sh", "ORIENTDB_PID=$ORIENTDB_HOME/bin", "ORIENTDB_PID=\"${ORIENTDB_PID:-#{var}/run/orientdb}\""

and then setting the environment variable in the test do block like this:

ENV["ORIENTDB_PID"] = testpath/"pid"

For the other two files that you need to manipulate for the regular installation, you'd need to check how to achieve this. Maybe you can copy the configuration files to the test path and then tell the server to use those files instead of the ones in the usual location.

@UniqMartin UniqMartin added the needs response Needs a response from the issue/PR author label Jun 20, 2016
@UniqMartin UniqMartin mentioned this pull request Jun 20, 2016
4 tasks
@SlamJam
Copy link

SlamJam commented Jun 23, 2016

@austinsmorris for successful passing of the tests use the following patch:

diff --git a/Formula/orientdb.rb b/Formula/orientdb.rb
index fb55488..48764c0 100644
--- a/Formula/orientdb.rb
+++ b/Formula/orientdb.rb
@@ -28,6 +28,7 @@ class Orientdb < Formula

     (var/"db/orientdb").mkpath
     (var/"log/orientdb").mkpath
+    (var/"run/orientdb").mkpath
     touch "#{var}/log/orientdb/orientdb.err"
     touch "#{var}/log/orientdb/orientdb.log"
     inreplace "#{libexec}/config/orientdb-server-config.xml", "</properties>", "  <entry name=\"server.database.path\" value=\"#{var}/db/orientdb\" />\n    </properties>"
@@ -46,8 +47,13 @@ class Orientdb < Formula
   end

   test do
+     ENV["CONFIG_FILE"] = "#{testpath}/orientdb-server-config.xml"
+
+     cp "#{libexec}/config/orientdb-server-config.xml", testpath
+     inreplace "#{testpath}/orientdb-server-config.xml", "</properties>", "  <entry name=\"server.database.path\" value=\"#{testpath}\" />\n    </properties>"
+
     system "#{bin}/orientdb", "start"
-    sleep 2
+    sleep 4

     begin
       assert_match "OrientDB Server v.2.2.2", shell_output("curl -I localhost:2480")

@ghost ghost removed the needs response Needs a response from the issue/PR author label Jun 23, 2016
@austinsmorris
Copy link
Contributor Author

Thanks @SlamJam, looks like it's good to go!

(var/"run/orientdb").mkpath
touch "#{var}/log/orientdb/orientdb.err"
touch "#{var}/log/orientdb/orientdb.log"
inreplace "#{libexec}/config/orientdb-server-config.xml", "</properties>", " <entry name=\"server.database.path\" value=\"#{var}/db/orientdb\" />\n </properties>"
Copy link
Member

Choose a reason for hiding this comment

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

Use EOS.undent instead of \ns. Split this into multiple lines so it wraps at ~80 characters.

@MikeMcQuaid
Copy link
Member

Last tiny nit and we should be good to go here 👍

@DomT4 DomT4 added the needs response Needs a response from the issue/PR author label Jul 6, 2016
@hayesdavis
Copy link

I'd love to see this get merged. Anything I could to do expedite the process?

@ghost ghost removed the needs response Needs a response from the issue/PR author label Jul 8, 2016
@dunn dunn added the needs response Needs a response from the issue/PR author label Jul 9, 2016
@MikeMcQuaid
Copy link
Member

@BrewTestBot test this please

@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Jul 10, 2016

Merged and then cleaned up in a795ebd so people could get using this sooner rather than later.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs response Needs a response from the issue/PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants