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

Automate matlab install #897

Merged
merged 12 commits into from
Jan 9, 2023
Merged

Conversation

JacobDGrunnet
Copy link
Contributor

Install script for automating acados installation using Matlab.

Also includes minor changes to enable full installation without user interaction.

Note: A few files were moved from examples to interfaces as they are actually needed to use acados and are thus part of the interface and not an example.

Furthermore, the include directory references were changes to point at the include directory created automatically when installing acados instead of directly from the source. This enables management of the acados install used on an enterprise level where the responsible software engineer can generate an acados package with a specific configuration and share a minimal and compiled acados distribution to the engineers using the solver generation.

Copy link
Member

@FreyJo FreyJo left a comment

Choose a reason for hiding this comment

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

Thanks! This seems convenient!

However, I do not want to break Octave compatibility here as well:
The tests are now failing with the error:
scalar cannot be indexed with .
Could you look into that and ideally fix it?

Moreover: Could you update the docs, to describe how the interface should be used after your proposed change?
Like: How to call the acados_install what are prerequisites to do so?

https://github.com/acados/acados/blob/master/docs/matlab_octave_interface/index.md

Jacob Deleuran Grunnet added 2 commits January 2, 2023 15:02
…s.m files from the example to the interface folder

This enables the use of the acados from Matlab not to depend on the examples, but instead be self contained in the interface folder

Additionally added option to force install of casadi such that the install can be run without user interaction.
…an be called from an install script

Additionally make it possible to force install to enable automation

# Conflicts:
#	interfaces/acados_matlab_octave/acados_template_mex/+acados_template_mex/render_acados_templates.m
@JacobDGrunnet
Copy link
Contributor Author

Rebased and now all tests passed here as well.

Be aware that I have been using the argument block in Matlab which was not introduced untill version 2019b

@FreyJo please have a look and if you are a happy with the approach I will update the index.md file as requested.

The automated installation works for windows with minGW following the guide given on the below URL

https://docs.acados.org/installation/index.html#windows-for-use-with-matlab

The above documentation seems more complete than the .md file.

Copy link
Member

@FreyJo FreyJo left a comment

Choose a reason for hiding this comment

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

I just went through the changes.

Be aware that I have been using the argument block in Matlab which was not introduced untill version 2019b

I would like to not make this restriction. There are people stuck with older Matlab versions/licenses I guess.
I see that arguments might be more readable, but would like to ask you to replace this using varargin.

The above documentation seems more complete than the .md file.

Seems like I linked the wrong file above.
It indeed makes more sense to document it here.
https://github.com/acados/acados/blob/master/docs/installation/index.md
Overall, the https://docs.acados.org page is built from the files in https://github.com/acados/acados/blob/master/docs

I'll try to test this new installation soon myself before merging.

Jacob Deleuran Grunnet added 6 commits January 3, 2023 21:54
@JacobDGrunnet
Copy link
Contributor Author

I just went through the changes.

Be aware that I have been using the argument block in Matlab which was not introduced untill version 2019b

I would like to not make this restriction. There are people stuck with older Matlab versions/licenses I guess. I see that arguments might be more readable, but would like to ask you to replace this using varargin.

It is a lot more readable :D

I have changed to varargin.

The above documentation seems more complete than the .md file.

Seems like I linked the wrong file above. It indeed makes more sense to document it here. https://github.com/acados/acados/blob/master/docs/installation/index.md Overall, the https://docs.acados.org page is built from the files in https://github.com/acados/acados/blob/master/docs

When you have approved the code-changes I will make and update to the documentation before you merge.

I'll try to test this new installation soon myself before merging.

@FreyJo
Copy link
Member

FreyJo commented Jan 5, 2023

I pushed a commit updating the acados_install_windows file.
Feel free to make the documentation commit before I test it myself, then I can directly merge.

Copy link
Member

@FreyJo FreyJo left a comment

Choose a reason for hiding this comment

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

A colleague of mine just tested it successfully.

The only pitfall was providing the acadosPath as an optional argument here:

    switch(nargin)
        case 0
            acadosPath=pwd;
        case 1
            acadosPath=varargin{1};

I think it could also be a non optional argument, or you specify how to use the script precisely in the docu to be added.
Otherwise, all good. Thanks! 🙌

@JacobDGrunnet
Copy link
Contributor Author

A colleague of mine just tested it successfully.

The only pitfall was providing the acadosPath as an optional argument here:

    switch(nargin)
        case 0
            acadosPath=pwd;
        case 1
            acadosPath=varargin{1};

I think it could also be a non optional argument, or you specify how to use the script precisely in the docu to be added. Otherwise, all good. Thanks! 🙌

I see. This was a choice I made when the install script was in the acados root :D

I would like to make a small update such that it is possible to specify custom cmake options as an optional input. However, I can make the acados path default path a lot smarter though....

Give me a little time and I will push a suggested change

@JacobDGrunnet
Copy link
Contributor Author

I added a smarter way of autodetecting the acados root folder (just relative to the locatio of the file)

Also it is now possible to add a user defined CMAKE configuration as a input parameter for the installs script

If this interface change is approved I can update the documentation

Copy link
Member

@FreyJo FreyJo left a comment

Choose a reason for hiding this comment

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

Given that you detect the acados path now, I would suggest to remove it as an argument from the function.
As it is now, the user needs to provide the path if they want to change the default CMake config, which is not so convenient.
Or is there a reason why you think the path detection fails?

Otherwise, it all looks good to me!

@JacobDGrunnet
Copy link
Contributor Author

JacobDGrunnet commented Jan 5, 2023

I kept the path as optional as I thought it might fail in the case where a developer has multiple acados repositories checked out.

However, on further reflection I do not think it can fail unless you have another acados installation in your Matlab path and I guess that failure in this case is a user error.

I submitted a commit removing this option and updating the documentation

…derivation of the acados root folder always works
@JacobDGrunnet
Copy link
Contributor Author

And the force push was to remove the bug I introduced :D

@FreyJo FreyJo merged commit 568e46c into acados:master Jan 9, 2023
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.

None yet

2 participants