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

AMIGAOS: Mutliple fixes #3854

Merged
merged 3 commits into from May 14, 2022
Merged

AMIGAOS: Mutliple fixes #3854

merged 3 commits into from May 14, 2022

Conversation

raziel-
Copy link
Contributor

@raziel- raziel- commented May 2, 2022

  • Add FS directory creation
  • Remove hardcoded newlib toolchain (there may be follow-up FR's)

@lephilousophe lephilousophe changed the title Patch 2 AMIGAOS: Mutliple fixes May 2, 2022
if (lock) {
IDOS->UnLock(lock);
debug("AmigaOSFilesystemNode::createDirectory() -> Directory '%s' already exists!", _sPath.c_str());
return _bIsValid && _bIsDirectory; // TODO: Should we make sure it's really a directory?
Copy link
Member

@bluegr bluegr May 2, 2022

Choose a reason for hiding this comment

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

I don't understand this comment. Is there a possibility that a node with _bIsDirectory = true is not a directory?

@sev-
Copy link
Member

@sev- sev- commented May 2, 2022

Could you please fix the commit log messages to have the proper "AMIGAOS: " prefixes?

@raziel-
Copy link
Contributor Author

@raziel- raziel- commented May 2, 2022

@sev-

I have no idea how to do that from github web page.
I can close and redo the PR?

raziel- added 2 commits May 3, 2022
- Remove hardcoded newlib toolchain (there´s an open-source clib2 toolchain
  available). Let the user decide which toolchain using LDFLAGS env var.
- Minor comment fixes.
@digitall
Copy link
Member

@digitall digitall commented May 3, 2022

@raziel- , @sev- : Should be fixed now. I pushed to fix the commit prefix and squash the trivial typo commit into the previous commit.

@raziel-
Copy link
Contributor Author

@raziel- raziel- commented May 4, 2022

@bluegr

Regarding the comment...this will be revised.

@capehill will (hopefully) join and help out, please stand by

@capehill
Copy link

@capehill capehill commented May 8, 2022

@raziel- and other devs

Is it so that before calling backend's createDirectory, FSNode actually checks the existence and type: https://github.com/scummvm/scummvm/blob/master/common/fs.cpp#L156

In that case, backend could be simplified to something like:

bool AmigaOSFilesystemNode::createDirectory() { 
    BPTR lock = IDOS->CreateDir(_sPath.c_str()); 
    if (lock) { 
        IDOS->UnLock(lock); 
        debug("AmigaOSFilesystemNode::createDirectory() -> Directory '%s' created", _sPath.c_str()); 
        _bIsValid = true; 
        _bIsDirectory = true; 
    } else { 
        warning("AmigaOSFilesystemNode::createDirectory() -> Failed to create '%s'", _sPath.c_str()); 
    }     
     
    return _bIsValid && _bIsDirectory; 
}

This is still untested.

- Change warnings to debug
- Fix trailing slash
@raziel-
Copy link
Contributor Author

@raziel- raziel- commented May 13, 2022

Would that be applicable?

Tested locally and working

@bluegr
Copy link
Member

@bluegr bluegr commented May 14, 2022

This looks good now, and is aligned with what other platforms do in the same function. Since the changes are working for you @raziel, they should be fine to merge.

Thanks for your work!

@bluegr bluegr merged commit a83ce7d into scummvm:master May 14, 2022
8 checks passed
@raziel- raziel- deleted the patch-2 branch May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants