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

Add Content file deployment to internal storage #814

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Ellerbach
Copy link
Member

@Ellerbach Ellerbach commented Mar 13, 2024

Description

Add Content file deployment to internal storage

** A new version of Debug lib needs to be published first **

Motivation and Context

How Has This Been Tested?

With test extension

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
  • I have added new tests to cover my changes.

if (contents.Any())
{
MessageCentre.InternalErrorWriteLine("Deploying content files to internal storage");
foreach(var file in contents)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
foreach(var file in contents)
foreach(var file in contents)

MessageCentre.InternalErrorWriteLine($"Deploying {file.EvaluatedInclude}");
if(file.EvaluatedInclude.Contains(Path.DirectorySeparatorChar))
{
MessageCentre.InternalErrorWriteLine("File should not be a path, internal storage does not support folders. It will still try to deploy.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MessageCentre.InternalErrorWriteLine("File should not be a path, internal storage does not support folders. It will still try to deploy.");
MessageCentre.InternalErrorWriteLine("File is included as a path, but internal storage does not support folders. Note: It will still try to deploy.");

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change is trying to convey the error rather than how to handle it (as convention), but if possible, an extra "suggestion" of how to handle the error would be advantagous.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change is trying to convey the error rather than how to handle it (as convention), but if possible, an extra "suggestion" of how to handle the error would be advantagous.

Yes, I was even thinking to replace the '' by a dash and write it. And also mention that in the docs. I'm open for any improvement on that side. Note that you can also have a look at the 2 other PR, the nf-interpreter one and the nf-debugger one where the core code is located.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this depends on what are the target capabilities. For ORGPAL boards (and others that offer options other than internal storage) these can be stored in SDCard or USB MSD, for example.
I would suggest a warning message with a generic warning about that being a path and the target device may not support that. Or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so we need an additional setting on the nfproj file to specify which drive to use. By default, in this draft implementation, it's using the internal storage.
Now: what is the interest of being able to deploy on a SDCard or external storage when it's much easier to do it another way?

Copy link
Member

Choose a reason for hiding this comment

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

Considering that this is target related, I would argue that this should go into user prefs and not project.
Apart from that, as this is the 1st pass, its more than OK to default to internal storage.

Copy link
Member

Choose a reason for hiding this comment

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

To your last question: that's, again, hardware dependent. The device may not have internal storage but may have an SDCard. That should be a perfectly valid storage for files being served to HTTP server or used as resources in any application...

Copy link
Member

Choose a reason for hiding this comment

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

How about File is included as a path, but operation does not allow this. Note: It will still try to deploy.

@josesimoes josesimoes force-pushed the add-storage-option branch 2 times, most recently from f9880ed to a9319e2 Compare March 21, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants