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 support for CONFIG_FS_WRITABLE #602

Merged
merged 12 commits into from
Mar 22, 2020

Conversation

patacongo
Copy link
Contributor

And also CONFIG_FS_READABLE.

This resolves issue #484 and is a companion to the PR 131 in incubator_nuttx_apps. Per Issue 484:

This is kind of a useless configuration variable that occurs in many places and adds a lot of complexity and makes debug a lot more difficult.

It makes debug more difficult because it is not consistently set and requires a lot of debug time before you finally figure out "I need to set CONFIG_FS_WRITABLE.

In most places affected by CONFIG_FS_WRITABLE, the configuration option is stupid: Who needs read-only MMC or read-only SmartFS. Just not a useful thing. And many places that do need CONFIG_FS_WRITABLE do not automatically set it. Like drivers/mtd/ftl.c. In that case, you can debug for most of the day before you finally figure out why the you can't access flash via character driver (which is a really common configuration for resource limited platforms).

It is hard to imagine a platform that supports a file system but has no writing enabled. I could imagine a system with only ROMFS or only CROMFS. But that is really about it and, in those cased, CONFIG_FS_WRITABLE has no effect. I am not aware of any such case, but that would be the only kind of platform that would suffer from the change.

But my opinion is that in the bigger picture, NuttX would be cleaner, less complex, and more usable with CONFIG_FS_WRITABLE removed.

CONFIG_FS_REABABLE was also becaue it does nothing.

@Ouss4 Ouss4 linked an issue Mar 21, 2020 that may be closed by this pull request
@patacongo
Copy link
Contributor Author

@xiaoxiang781216 This is currently failing the same sim checks that is blocking your PR.

However, if we get past that, I don't think it will be able to build the checks. I will verify that any failed configurations work on my PC, but I do not expected the nutx/ changes to build correctly without the apps/ changes or vice versa.

What do you recommend?

@patacongo
Copy link
Contributor Author

@xiaoxiang781216 yes, I verfied on apps/ PR 131 that the failing builds workd perfectly will when both the apps/ and nuttx/ changes are in place. How, do you propose that we verify this?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Mar 22, 2020

@xiaoxiang781216 yes, I verfied on apps/ PR 131 that the failing builds workd perfectly will when both the apps/ and nuttx/ changes are in place. How, do you propose that we verify this?

Let's bypass the precheck system before it can handle the PR relationship in apps/nuttx.
@papatience, do you want fix the rest nxstyle issue in apache/nuttx-apps#131?

@xiaoxiang781216 xiaoxiang781216 merged commit 547a3cb into apache:master Mar 22, 2020
@patacongo patacongo deleted the writable branch March 22, 2020 14:48
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.

Remove CONFIG_FS_WRITABLE
3 participants