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

Build CUTEst inside deps/ #104

Closed

Conversation

timholy
Copy link
Contributor

@timholy timholy commented Nov 22, 2016

The first commit downloads the files into a directory deps/files. After checking for a "global" installation (ref #103 (comment)), it then checks for a "local" (julia-specific) install. Only if the latter is not found does it download and install the files. EDIT: this will avoid unnecessary re-installations whenever you tag a new version of CUTEst (Pkg.build runs every time a package gets updated).

The second commit may be more controversial. It adds a new environment variable, CUTEst problems which points to the directory deps/files/problems. It leverages this to build all the *.so files (and the AUTOMAT.d and OUTSDIF.d files) inside this folder, rather than the user's current folder. I've been deleting .so files from various folders that I run CUTEst problems in, so centralizing these files seems like a nice way to prevent clutter. On the other hand, I wonder if it's currently possible to have two different julia sessions running in two different directories, both working on CUTEst problems; if this gets centralized, that won't be possible anymore (I presume). If so, I guess one option would be to build them in a temporary folder, and have each julia session generate a different random temporary.

@timholy
Copy link
Contributor Author

timholy commented Nov 22, 2016

I should add, this fixes #103.

@coveralls
Copy link

coveralls commented Nov 22, 2016

Coverage Status

Coverage increased (+0.09%) to 62.388% when pulling 89d6102 on timholy:pull-request/11e3b92a into f1f853f on JuliaSmoothOptimizers:develop.

@timholy
Copy link
Contributor Author

timholy commented Nov 22, 2016

The OSX errors arise from installation (click to expand the julia -E 'Pkg.build("CUTEst");'), specifically a Error: no implicit conversion of nil into String apparently in the Homebrew ruby code. I don't think that's related to this PR, but perhaps someone more knowledgeable than me about OSX should take a peek?

@dpo
Copy link
Member

dpo commented Nov 22, 2016

Must be a recent change in Homebrew. I'll have a look asap.

@dpo
Copy link
Member

dpo commented Dec 9, 2016

I am wondering if the issue comes from Travis/OSX because I can't reproduce locally. I "fixed" the Homebrew formula for CUTEst to make Travis happy and now it installs, but there's an error further down, in the tests. I can't reproduce them locally either.

@dpo dpo mentioned this pull request Dec 9, 2016
@abelsiqueira
Copy link
Member

I restarted CUTEst develop branch and it works for 0.4 and 0.5. There is something here that is breaking OSX. Look at this line. It shows that CUTEst on OSX was installed on a tmp dir.

@@ -10,14 +10,15 @@ function validate_libcutest()
tmpdir = mktempdir()
cd(tmpdir)
Copy link
Member

Choose a reason for hiding this comment

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

This line is not needed, line 13 already calls it.

end
end
println(cenv, "ENV[\"CUTEst problems\"] = \"$(pwd())/problems\"")
Copy link
Member

Choose a reason for hiding this comment

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

This pwd() is possibly wrong.

run(`wget $lnxurl -O install.sh`)
run(`bash install.sh --install-deps`)
@static if is_linux()
cd(here) do
Copy link
Member

Choose a reason for hiding this comment

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

This is missing for OSX

path = chomp(split(line, "=")[2])
write(cenv, "ENV[\"$var\"] = \"$path\"\n")
end
println(cenv, "ENV[\"CUTEst problems\"] = \"$(pwd())/problems\"")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary; all problems in that folder are already in ENV["MASTSIF"].

Copy link
Member

Choose a reason for hiding this comment

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

This is for the .so, .f and .d files, I believe, so that the workdir remains clear.

@abelsiqueira
Copy link
Member

I made a debug commit and indeed the problem is what's mentioned above: The cd(tmpdir) alone and the wrong pwd(). @timholy, do you wanna fix these, or can I merge locally and fix? @dpo, do you agree? I would then push directly after.

@dpo
Copy link
Member

dpo commented Dec 14, 2016

Yes, that seems right. Thanks @abelsiqueira!

@abelsiqueira
Copy link
Member

Pushed with fix. Thanks @timholy .

@timholy timholy deleted the pull-request/11e3b92a branch January 5, 2017 18:43
@timholy
Copy link
Contributor Author

timholy commented Jan 5, 2017

Thanks for implementing the fix! I got busy with other things.

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.

4 participants