Remove git submodule in favor of CMake commands #817

oxtopus · 2014-04-15T22:42:15Z

Fixes #798.
Fixes #634.

Removed nta git submodule, replaced with CMake commands to manually clone, fetch, and checkout nupic.core repository. This also allows overriding the defaults on the command line via NUPIC_CORE_REMOTE, for the remote repository url, and NUPIC_CORE_COMMITISH, for the specific sha.

There are several advantages to this approach:

No git submodules (nupic git repository is no longer explicitly coupled to git://github.com/numenta/nupic.core.git)
~~CMakeLists.txt~~ .nupic_modules is the ground truth for nupic.core version dependency
User can override NUPIC_CORE_REMOTE and NUPIC_CORE_COMMITISH at-will, if they are not using the official nupic.core repository.
Developer can make changes in nta/ and push to remote repository of their choosing

…lone, fetch, and checkout nupic.core repository. This also allows overriding the defaults on the command line via NUPIC_CORE_REMOTE, for the remote repository url, and NUPIC_CORE_COMMITISH, for the specific sha.

oxtopus · 2014-04-15T22:44:35Z

Currently running through Numenta internal pipelines to understand impact.

breznak · 2014-04-15T23:38:07Z

👍 very nice! thanks @oxtopus

utensil · 2014-04-16T07:37:35Z

I've read the code in the PR, and realize that while I'm +1 for the idea to let CMake clone nupic and update to a specific commit based on something, but I would -1 for the something(direct set in CMakefile) used here, reasoning:

CMakefile is not in .gitignore, changing it for the purpose of building against an unstable development might be error-prone and cause dirty commit.

Here's my 2c:

CMake read something that commits into nupic to determine the default repo and SHA1
CMake read something optional that is in .gitignore, to determine an overwritten repo and SHA1

breznak · 2014-04-16T07:43:16Z

@utensil I think the point is that you can export the variables with new values (w/o changes to CMakeLists.txt) and thus change the behavior.

@oxtopus will those exported values be overridden every time I call cmake .. ? (so I need to export them again?)

utensil · 2014-04-16T08:05:25Z

@breznak I understand the use of env vars here, but it confuses me, because aren't we trying to reduce them?

Expand a little more:

default repo and SHA1 is better outside CMakeFile.txt, seperate it from CMake logics.
instead of using env vars, I prefer a config file to do so, because if I'm working with numenta/nupic, david-ragazzi/nupic and utensil/nupic, I may maintain different values of repo and SHA1 local to each fork .

breznak · 2014-04-16T08:12:32Z

instead of using env vars, I prefer a config file to do so, because if
I'm working with numenta/nupic, david-ragazzi/nupic and utensil/nupic, I may
maintain different values of repo and SHA1 local to each fork .

I see, this one would be nice feature. (would need a warning though,
because I can imagine the forgotten values there leading to weird errors)

oxtopus · 2014-04-16T13:15:53Z

My intention is not to use env variables, but instead allow the user to
override via CLI option. If this option allows for both, then I would
consider it a bonus!

I'm a total noob with CMake, so I'm open to suggestions.

On Wednesday, April 16, 2014, breznak notifications@github.com wrote:

instead of using env vars, I prefer a config file to do so, because if
I'm working with numenta/nupic, DavidRagazzi/nupic and utensil/nupic, I may
maintain different values of repo and SHA1 local to each fork .

I see, this one would be nice feature. (would need a warning though,
because I can imagine the forgotten values there leading to weird errors)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/817#issuecomment-40573808
.

oxtopus · 2014-04-16T13:37:36Z

To further clarify, the user need not modify CMakeLists.txt in order to test alternative forks or commits. The idea is the user would run cmake with additional option:

cmake -DNUPIC_CORE_COMMITISH:STRING=b2727fb8f8d38221f35038c638916702cd434e57 $NUPIC
cmake -DNUPIC_CORE_COMMITISH:STRING=HEAD $NUPIC
cmake -DNUPIC_CORE_COMMITISH:STRING=feature-branch $NUPIC
cmake -DNUPIC_CORE_COMMITISH:STRING=origin/master -DNUPIC_CORE_REMOTE:STRING=/some/arbitrary/url $NUPIC

When they're ready to push those changes upstream, then they modify CMakeLists.txt and submit a PR.

utensil · 2014-04-16T13:58:15Z

To further clarify, the user need not modify CMakeLists.txt in order to test alternative forks or commits. The idea is the user would run cmake with additional option

Despite of my preferences to config(which would reduce repetitive typing or using the upper arrow) stated above, this PR would definitely help and improve development productivity in a way I can also adapt to.

When they're ready to push those changes upstream, then they modify CMakeLists.txt and submit a PR.

Still, a separate file(other than CMakeLists.txt ) to specify nupic.core's commitish look more clear to me.

david-ragazzi · 2014-04-16T14:53:13Z

The idea is the user would run cmake with additional option:

cmake -DNUPIC_CORE_COMMITISH:STRING=b2727fb8f8d38221f35038c638916702cd434e57 $NUPIC
cmake -DNUPIC_CORE_COMMITISH:STRING=HEAD $NUPIC
cmake -DNUPIC_CORE_COMMITISH:STRING=feature-branch $NUPIC
cmake -DNUPIC_CORE_COMMITISH:STRING=origin/master -DNUPIC_CORE_REMOTE:STRING=/some/arbitrary/url $NUPIC

+1

david-ragazzi · 2014-04-16T14:58:34Z

CMakeLists.txt

+      message(FATAL_ERROR "Unable to fetch ${NUPIC_CORE_REMOTE}")
+    endif()
+  endif()
+  execute_process(COMMAND git checkout ${NUPIC_CORE_COMMITISH}


Consider use a parameter variable like ${commitish} instead of ${NUPIC_CORE_COMMITISH}

oxtopus · 2014-04-16T15:28:41Z

@utensil Can you provide an example for how I might move the NUPIC_CORE_REMOTE and NUPIC_CORE_COMMITISH into a separate file?

Would you approve this PR as-is, assuming that we could follow up with the separate file idea in a different PR?

utensil · 2014-04-16T15:31:33Z

I'm OK with current PR as is, we may investigate the idea in another PR.

breznak · 2014-04-16T15:35:15Z

Would you approve this PR as-is, assuming that we could follow up with
the separate file idea in a different PR?

+100

david-ragazzi · 2014-04-16T15:48:49Z

CMakeLists.txt

+  # checkout git dependency
+  if (NOT EXISTS ${submodule_dir})
+    execute_process(COMMAND git clone ${remote} ${submodule_dir})
+    if(NOT EXIT_CODE EQUAL 0)


You forgot add RESULT_VARIABLE EXIT_CODE option to execute_process command.. On the contrary, you will use a empty or wrong value..

Do this:

execute_process(COMMAND git clone ${remote} ${submodule_dir} RESULT_VARIABLE EXIT_CODE)

Fixed. Thanks!

david-ragazzi · 2014-04-16T16:00:19Z

.gitmodules

@@ -1,3 +0,0 @@
-[submodule "nupic.core"]


Should we keep this file in root yet? How about we delete it once (of course, if Travis get green with this removal)?

The less files the better! 😃

If it's empty, we should remove it. I just wasn't sure if it would cause problems so I left it empty.

Yes, it's empty.. I don't see problems in remove it...

Good job, bro! This PR is really useful!

david-ragazzi · 2014-04-17T18:23:07Z

Ok, I merged as of 7d7a6fe. I had to manually resolve some conflicts, so you can close the PR.

Thanks! It already was closed.. hehe

rhyolight · 2014-04-20T15:35:48Z

CMakeLists.txt

@@ -29,6 +29,51 @@
 ###                                                                                                                      ###
 ############################################################################################################################

+# This function read variables from a file in the following format:


Typo: This function reads

rhyolight · 2014-04-20T15:48:39Z

@oxtopus We seem to have a Travis problem. The last push did not trigger a Travis-CI build. The only way I know to trigger one is to push another commit.

Otherwise, 👍 Looks great!

oxtopus · 2014-04-20T15:55:28Z

I don't know what's up w/ travis and the reported status. https://travis-ci.org/oxtopus/nupic/builds/23225301, which is the latest commit in this branch is green, and #830 is green, which is a superset of this and only differs in Dockerfile, which wouldn't affect the travis build.

@rhyolight are there any implications on tooling wrt nupic and nupic.core assuming this is merged?

…ents, and (hopefully) trigger another travis build to get the status green

rhyolight · 2014-04-21T15:03:15Z

@oxtopus We are green and good to go. Before the final merge, are there any README changes that need to be made? Do we need to inform our current users of this at all?

oxtopus · 2014-04-21T15:16:54Z

@rhyolight Nothing needed in the README. I've played through a few scenarios of switching between different SHAs before and after without any problem. The required changes are encapsulated within the changeset, so it will apply cleanly. The only problems someone might encounter is if they were modifying nta/ inline and deviated from the instructions. In which case the merge will fail for them and they will have to resolve the conflict on their own.

I will send a heads up to nupic-hackers.

Remove git submodule in favor of CMake commands

rhyolight · 2014-04-21T15:32:21Z

@oxtopus Thanks. Any chance you could add a wiki page with instructions on how to create the appropriate local config files to customize the remote location and committish of nupic.core? If you make it, I'll link in in the appropriate places.

oxtopus · 2014-04-21T15:38:00Z

Sure thing.

oxtopus · 2014-04-21T16:19:50Z

@rhyolight here you go: https://github.com/numenta/nupic/wiki/nupic.core-dependencies

rhyolight · 2014-04-21T16:41:56Z

Beautiful, thanks.

Matt Taylor
OS Community Flag-Bearer
Numenta

On Mon, Apr 21, 2014 at 9:19 AM, Austin Marshall
notifications@github.comwrote:

@rhyolight https://github.com/rhyolight here you go:
https://github.com/numenta/nupic/wiki/nupic.core-dependencies

Reply to this email directly or view it on GitHubhttps://github.com//pull/817#issuecomment-40948467
.

rhyolight · 2014-04-21T16:46:32Z

BTW, I changed the wiki name to "NuPIC's-Dependency-on-nupic.core" and
linked it from
https://github.com/numenta/nupic/wiki/Contributing-to-NuPIC#other-references(for
now).

Matt Taylor
OS Community Flag-Bearer
Numenta

On Mon, Apr 21, 2014 at 9:41 AM, Matthew Taylor matt@numenta.org wrote:

Beautiful, thanks.

Matt Taylor
OS Community Flag-Bearer
Numenta

On Mon, Apr 21, 2014 at 9:19 AM, Austin Marshall <notifications@github.com

wrote:

@rhyolight https://github.com/rhyolight here you go:
https://github.com/numenta/nupic/wiki/nupic.core-dependencies

Reply to this email directly or view it on GitHubhttps://github.com//pull/817#issuecomment-40948467
.

utensil · 2014-04-22T10:17:40Z

👍 for the work and the wiki . Really excellent and helpful, can't wait to test it.

breznak · 2014-04-22T18:28:51Z

KUDOs @oxtopus, this is great!

The build is rocking with my new neat settings for local devel:

Default nupic.core dependencies (override in optional .nupic_modules)

NUPIC_CORE_REMOTE = '/home/nupic/nupic.core'
NUPIC_CORE_COMMITISH = 'master/HEAD'

PS: nitpicks: I think it should be

(override in optional .nupic_modules)
(override in optional .nupic_config)

and .nupic_config should be in gitignore?

Thanks a lot! :)

breznak · 2014-04-22T18:31:33Z

one bug, it seems to generate makefiles in $NUPIC, not $NUPIC/build, or
$PWD, as it should (?)

On Tue, Apr 22, 2014 at 8:28 PM, Marek Otahal markotahal@gmail.com wrote:

KUDOs @oxtopus, this is great!

The build is rocking with my new neat settings for local devel:

Default nupic.core dependencies (override in optional .nupic_modules)

NUPIC_CORE_REMOTE = '/home/nupic/nupic.core'
NUPIC_CORE_COMMITISH = 'master/HEAD'

PS: nitpicks: I think it should be

(override in optional .nupic_modules)

(override in optional .nupic_config)

and .nupic_config should be in gitignore?

Thanks a lot! :)

Marek Otahal :o)

oxtopus · 2014-04-23T00:05:25Z

@breznak Oops! You're right re: comment in .nupic_modules and overrides. Will follow up.

oxtopus · 2014-04-23T00:16:43Z

@breznak

Starting from a clean state:

$ find $NUPIC -name Makefile
/Users/amarshall/nta/nupic/lang/c#/Makefile
/Users/amarshall/nta/nupic/lang/java/Makefile

Then, after building:

$ find $NUPIC -name Makefile
/Users/amarshall/nta/nupic/build/scripts/Makefile
/Users/amarshall/nta/nupic/lang/c#/Makefile
/Users/amarshall/nta/nupic/lang/java/Makefile
/Users/amarshall/nta/nupic/nta/build/scripts/Makefile

Are you seeing something different?

chetan51 · 2014-04-23T05:13:01Z

Hmm, I tried putting a .nupic_config file in my NuPIC directory, and in my home directory, but it seems to have no effect. Am I doing something wrong?

chetan51 · 2014-04-24T01:09:30Z

Reading the CMakeLists.txt file, it looks like the .nupic_config file isn't really considered. Can someone confirm this?

oxtopus · 2014-04-24T01:34:18Z

I think you're right. Only place the function is used is https://github.com/numenta/nupic/blob/master/CMakeLists.txt#L600

How'd we miss that?

oxtopus · 2014-04-24T01:39:53Z

See #849 for possible fix. Works for me, at least.

oxtopus added 2 commits April 15, 2014 14:47

Remove nupic.core entry from .gitmodules

171e4ad

Specify WORKING_DIRECTORY in git fetch

0202ebe

oxtopus mentioned this pull request Apr 15, 2014

Build option to allow custom nupic.core submodule #798

Closed

oxtopus changed the title ~~Remove git submodule in favor of CMake control~~ Remove git submodule in favor of CMake commands Apr 15, 2014

This was referenced Apr 16, 2014

SHA of nupic.core, submodule in CMakeList, move -> src/ #767

Closed

Update CMake to update submodule #661

Closed

lower-case "set" for consistency

0f46dd6

david-ragazzi reviewed Apr 16, 2014
View reviewed changes

Updated with feedback in numenta#817

a6664fc

david-ragazzi reviewed Apr 16, 2014
View reviewed changes

Same result to EXIT_CODE variable

894e8c1

david-ragazzi reviewed Apr 16, 2014
View reviewed changes

oxtopus mentioned this pull request Apr 19, 2014

Create trusted build in Docker index and remove numenta/docker-nupic #634

Closed

utensil mentioned this pull request Apr 19, 2014

rename source and SWIG files to follow UpperCamelCase convention #829

Closed

oxtopus mentioned this pull request Apr 19, 2014

Updated Dockerfile #830

Merged

rhyolight reviewed Apr 20, 2014
View reviewed changes

rhyolight added this to the Sprint 20 milestone Apr 20, 2014

rhyolight assigned oxtopus Apr 20, 2014

Clarify .nupic_config and .nupic_modules files in CMakeLists.txt comm…

f7631d2

…ents, and (hopefully) trigger another travis build to get the status green

oxtopus added a commit that referenced this pull request Apr 21, 2014

Merge pull request #817 from oxtopus/remove-git-submodule

7689bc8

Remove git submodule in favor of CMake commands

oxtopus merged commit 7689bc8 into numenta:master Apr 21, 2014

oxtopus mentioned this pull request Apr 23, 2014

.nupic_config nitpicks #844

Merged

Remove git submodule in favor of CMake commands #817

Remove git submodule in favor of CMake commands #817

Conversation

oxtopus commented Apr 15, 2014

oxtopus commented Apr 15, 2014

breznak commented Apr 15, 2014

utensil commented Apr 16, 2014

breznak commented Apr 16, 2014

utensil commented Apr 16, 2014

breznak commented Apr 16, 2014

oxtopus commented Apr 16, 2014

oxtopus commented Apr 16, 2014

utensil commented Apr 16, 2014

david-ragazzi commented Apr 16, 2014

Choose a reason for hiding this comment

oxtopus commented Apr 16, 2014

utensil commented Apr 16, 2014

breznak commented Apr 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-ragazzi commented Apr 17, 2014

Choose a reason for hiding this comment

rhyolight commented Apr 20, 2014

oxtopus commented Apr 20, 2014

rhyolight commented Apr 21, 2014

oxtopus commented Apr 21, 2014

rhyolight commented Apr 21, 2014

oxtopus commented Apr 21, 2014

oxtopus commented Apr 21, 2014

rhyolight commented Apr 21, 2014

rhyolight commented Apr 21, 2014

utensil commented Apr 22, 2014

breznak commented Apr 22, 2014

Default nupic.core dependencies (override in optional .nupic_modules)

breznak commented Apr 22, 2014

Default nupic.core dependencies (override in optional .nupic_modules)

oxtopus commented Apr 23, 2014

oxtopus commented Apr 23, 2014

chetan51 commented Apr 23, 2014

chetan51 commented Apr 24, 2014

oxtopus commented Apr 24, 2014

oxtopus commented Apr 24, 2014