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

Switch to using Conda binaries #218

Closed
wants to merge 16 commits into from
Closed

Switch to using Conda binaries #218

wants to merge 16 commits into from

Conversation

odow
Copy link
Member

@odow odow commented Feb 28, 2024

Xpress publish binaries on Conda: https://anaconda.org/fico-xpress/xpress/files

They include a community license. If this works, it would greatly simplify things, and we'd be using their official binaries.

Closes #217

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.67%. Comparing base (be5e6f3) to head (926a8d9).
Report is 84 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
- Coverage   56.64%   49.67%   -6.97%     
==========================================
  Files           9        9              
  Lines        4288     4815     +527     
==========================================
- Hits         2429     2392      -37     
- Misses       1859     2423     +564     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odow
Copy link
Member Author

odow commented Feb 29, 2024

@joaquim should I add Windows too? You'll probably want to test this with your PSR binaries and license before we merge.

@odow
Copy link
Member Author

odow commented Feb 29, 2024

Windows support seemed problematic:

image

But I don't have a local Windows to debug.

@hellemo
Copy link
Contributor

hellemo commented Feb 29, 2024

Very nice! FWIW I just did a quick test in Windows, and it seems to work perfectly (as a user, did not test CI).

One caveat is that this is not a pristine system (just renamed my xpress folder), and the license file was found automatically. I suspect this might be from a previous install. What is the recommended way to provide the location of the license file when binaries are provided automatically?

@odow
Copy link
Member Author

odow commented Feb 29, 2024

I just did a quick test in Windows

Did you use one of the intermediate commits? I removed automatic from the latest commit.

We need to document, but it would be to set the XPAUTH_PATH pointing to the license file

@hellemo
Copy link
Contributor

hellemo commented Feb 29, 2024

Weird, now it doesn't work anymore. I think maybe I was relying on the results from a previous Pkg.build for it to work, and switching between this branch and the release version didn't trigger a rebuild.

When I lost contact with the license server, I did a manual build and since then it doesn't work anymore. (the need to rebuild after losing the license server is rather annoying).

@hellemo
Copy link
Contributor

hellemo commented Feb 29, 2024

I think I figured it out, I reenabled the artifact for windows, then I get the same permissions error as you mentioned in an earlier comment.
If I also set executable permissions to the library directory, I get it to work as expected:

elseif Sys.iswindows()  # Use the artifact instead.
    const xpressdlpath = joinpath(
        LazyArtifacts.artifact"xpresspy",
        joinpath("Lib", "site-packages", "xpress", "lib"),
    )
    chmod(dirname(xpressdlpath), 0o755; recursive=true) 
    const libxprs = joinpath(xpressdlpath, "xprs.dll")

@odow
Copy link
Member Author

odow commented Feb 29, 2024

Oh nice! Let me try that

@odow
Copy link
Member Author

odow commented Feb 29, 2024

Niiiiice.

One thing was talking to @simonbowly about. I wonder if it makes sense to split the binaries into an Xpress_jll.jl package. Then we can version the upstream binaries separately from Xpress.jl.

Otherwise everytime we change the Artifacts.toml it's a potential breaking change to Xpress.jl.

@odow odow mentioned this pull request Mar 1, 2024
1 task
@odow
Copy link
Member Author

odow commented Mar 1, 2024

See #220

That might take a while to tag and release etc, so we should do this PR first.

That'll let me make some progress on open issues, and we can decide what to do before we tag the next release.

@joaquimg
Copy link
Member

joaquimg commented Mar 1, 2024

I will test here to

@joaquimg
Copy link
Member

joaquimg commented Mar 1, 2024

I am not sure about the consequences of adding the artifacts.

Maybe a weakdep on Xpress_jll?

@odow
Copy link
Member Author

odow commented Mar 1, 2024

The artifacts are lazy, so they should only be downloaded if you use them.

@odow
Copy link
Member Author

odow commented Mar 1, 2024

I think ideally, we'd all just use Xpress_jll and Gurobi_jll, and version them appropriately. No more installation issues.

@odow
Copy link
Member Author

odow commented Mar 5, 2024

@joaquimg can you try this branch? It should use the binaries only if CI is true.

@odow odow mentioned this pull request Mar 7, 2024
@odow
Copy link
Member Author

odow commented Mar 7, 2024

Closing in favor of #221

@odow odow closed this Mar 7, 2024
@odow odow deleted the od/use-conda-binaries branch March 10, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Renew CI license
3 participants