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

remove AddToFileServer() #76

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

axel-h
Copy link
Member

@axel-h axel-h commented Mar 23, 2023

This PR is on top of #31 and removes the CMake function AddToFileServer(). It is not longer needed because DefineCAmkESVMFileServer() can take files and list of files as parameter now. Then CMake lists can be used to build the CPIO content.

In seL4/camkes-vm-examples#37 all calls to AddToFileServer() are removed. But I assume there are 3rd party systems out there that also use this function. So, as an intermediate step, we could have AddToFileServer() print a deprecation warning for the next months.

Test with: seL4/camkes-vm-examples#37, seL4/seL4_tools#151

@kent-mcleod
Copy link
Member

Is there an argument for why AddToFileServer() should be removed? It's a breaking change and also the ability to add files to the file server at different points in the build scripts is a useful feature.

@axel-h
Copy link
Member Author

axel-h commented Mar 30, 2023

It is no longer needed, because an application can now simply build a CMake list with the files. It can manipulate the list any time. Details are fully unter application control, until eventually the final list is passed to DefineCAmkESVMFileServer(), which is called in the end when DeclareCAmkESRootserver()is called.
There seems no much benefit if this appending to a list is wrapped in a function, and I also doubt this is a real convenience improvement. Also, from the changes you can see things get much simpler internally then. We do not have to maintain state variables over calls. I thing that makes it worth changing this. But as you said, the downside is a breaking change. So we may want to have deprecation period to give people time to change things. But the change is quite trivial, as you can see from the VM examples.

@kent-mcleod
Copy link
Member

It is no longer needed, because an application can now simply build a CMake list with the files.

The current way doesn't require an application to manage the list itself, it simply has to call the function each time it wants to add files. This is a common pattern that's all throughout CMake and is also followed by the camkes-tool CMake modules as well.

As far as I know there's no strong reason why the existing implementation is insufficient. Maybe there are some complicated apps that want to do more complicated build time processing and mutate the list of files in the file server after they've been added but these apps would be free to manage their own list before declaring the fileserver at the end also.

@axel-h
Copy link
Member Author

axel-h commented Apr 10, 2023

I can see you point and agree that it is convenient for applications. I can also see that having the API AddToFileServer() can have some advantages, and keeping it causes no harm.
However, this creates some internal complexity and corner cases, which I think could be avoided in the long run. It's much better easier dealt with on the application level. I also consider change of using the function to managing a list to be trivial CMake usage, so it seems not really worth having another layer here. Unless, there more that AddToFileServer() could do, e.g. more file type specific transformation convenience functions But even these might be better kept agnostic of this specific file server then and just be intermediate CMake targets.

@axel-h axel-h force-pushed the patch-axel-26 branch 5 times, most recently from b5d6d67 to 747533d Compare April 12, 2023 23:44
@axel-h axel-h force-pushed the patch-axel-26 branch 3 times, most recently from 1e9ccd2 to ce225ba Compare April 20, 2023 12:11
@axel-h axel-h added the hw-test camkes-vm-examples hardware builds + runs for this PR label Apr 28, 2023
@axel-h axel-h force-pushed the patch-axel-26 branch 3 times, most recently from b9db776 to 169268e Compare May 5, 2023 16:52
@axel-h
Copy link
Member Author

axel-h commented May 5, 2023

Turns out we can also remove the convenience wrapper DeclareCAmkESVMRootServer() then, as this is no longer used. My long term suggestion would be deprecating and removing this also to get rid of convenience layers that seem not really helpful because they hide too much.

Axel Heider added 3 commits January 31, 2024 13:57
With the new interface of DefineCAmkESVMFileServer() there is not need
to have AddToFileServer(). CMake lists with files can be built instead
and then passed to DefineCAmkESVMFileServer().

Signed-off-by: Axel Heider <[email protected]>
Remove convenience wrapper, as it is no longer useful.

Signed-off-by: Axel Heider <[email protected]>
@axel-h axel-h added hw-test camkes-vm-examples hardware builds + runs for this PR and removed hw-test camkes-vm-examples hardware builds + runs for this PR labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hw-test camkes-vm-examples hardware builds + runs for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants