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: Cleanup #2967

Merged
merged 19 commits into from May 1, 2021
Merged

AMIGAOS: Cleanup #2967

merged 19 commits into from May 1, 2021

Conversation

@raziel-
Copy link
Contributor

@raziel- raziel- commented Apr 19, 2021

amigaos-fs.cpp

  • Cleaned up comments
  • Debug messages enhanced
  • Fix some leftover leading spaces instead of TABs
  • Removed newlib workaround: A new version was released, which fixed the cause
  • Removed an old workaround dealing with a too low buffer due to an ill combination of a newlib build and a specific disk fs
    Even forcing the formerly described behaviour, i was not able to see slowdowns in videos
    Also, since "slow video playback" is rather vague and no specific case were given in the comment, i wasn´t able to do specific testing. If i ever encounter a regressive slowdown in video playback, i hope to remember this workaround.
    Works for me...

configure

  • Change absolute install path to user-definable assign
  • Change output of system dialogs to "asl" for both amigaos and morphos, since both targets use the same dialog system

ext_inst_so.rexx

  • Changes to make the script work with the new install dir
  • Cleanup to commands

rm2ag.rexx

  • Cleanup to commands

Note: I know i touched MorphOS related stuff in configure, but it´s just textual output, so i hope this is ok

raziel- added 6 commits Apr 19, 2021
- Change install path to use an assign instead of an absolute path
- Change system dialogs output to print "asl" instead of "amigaos (asl)", incorporated with morphos (since both targets use the same dialog system) to save some lines
Also janitorial
- Cleaned up comments
- Debug messages enhanced
- Fix some leftover spaces instead of TABs
- Removed the newlib workaround: A new version has been released which fixed the cause
- Removed an old workaround dealing with a buffer set too low due to an ill combination of newlib build and disk filesystem.
Even forcing the described behaviour i was not able to see slowdowns in videos.
Also, since "slow video playback" is rather vague and no specific case were given in the comment, i wasn´t able to do specific tesing.

Works for me...
@raziel-
Copy link
Contributor Author

@raziel- raziel- commented Apr 19, 2021

More testing needed

@raziel- raziel- closed this Apr 19, 2021
@BeWorld2018
Copy link
Contributor

@BeWorld2018 BeWorld2018 commented Apr 19, 2021

no problem here, thanks

raziel- added 2 commits Apr 20, 2021
@raziel-
Copy link
Contributor Author

@raziel- raziel- commented Apr 20, 2021

@BeWorld2018

Thank you for the feedback

@raziel- raziel- reopened this Apr 20, 2021
@lephilousophe lephilousophe self-assigned this Apr 20, 2021
@lephilousophe lephilousophe self-requested a review Apr 20, 2021
@@ -228,7 +228,7 @@ _dwp=dwp
_windres=windres
_stagingpath="staging"
_win32path="c:/scummvm"
_amigaospath="Games:ScummVM"

This comment has been minimized.

@lephilousophe

lephilousophe Apr 24, 2021
Member

This change breaks badly buildbot (and building on Unix more generally) because : has no meaning here and is not a path separator.
Besides, in backends/platform/sdl/amigaos/amigaos.mk, there is a:
cp ${srcdir}/dists/amigaos/scummvm_drawer.info $(AMIGAOSPATH).info which would generate a .info file on AmigaOS. Is it intended?
Finally, when building the final package, what is the directory expected in zip file? ScummVM: one?

This comment has been minimized.

@raziel-

raziel- Apr 24, 2021
Author Contributor

@lephilousophe

Damn...I thought I had found a way to make building more generic for others and not as personal as it is now.
Guess I'll have to revert it then.
Will take some time though, not near my gear.

Wrt the .info file.
Yes, that would be intended behaviour.
AmigaOS works with .info files for drawers and apps. They are distinguished Icons with information for every single file (which can be personalized to your hearts content).
Due to the assign (that's why the path ends on a : ) it would have been possible for anyone to set it to a destination they like and not be forced to gave a games:scummvm destination anywhere on their system.
The produced .info in this case is a drawer icon that would automatically be placed above the subdir (to whatever subdirectory the user has set the assign to).

But if it breaks so badly I guess i gave to live with it.
Thank you for the feedback.

Right now, the .zip would produce a games/scummvm subdue, with the assign it would be reduced to a games subdir (at least thats what I was hoping)

This comment has been minimized.

@lephilousophe

lephilousophe Apr 25, 2021
Member

I think I didn't explain well what I noticed with .Info file.
The way it is now implies that the command cp ${srcdir}/dists/amigaos/scummvm_drawer.info $(AMIGAOSPATH).info will get translated to something like cp ${srcdir}/dists/amigaos/scummvm_drawer.info ScummVM:.info and should result with an unnamed .Info placed inside ScummVM assigned directory and not alongside it.

For the : stuff, maybe we should make _amigaospath configurable? I think it would be the simpler for everyone.

This comment has been minimized.

@raziel-

raziel- Apr 25, 2021
Author Contributor

Normally, the assign would be translated to a correct path locally.
On the linux build It would probably create a mess, not sure what a .info file would be handled afterwards.

If that configuration option is possible, I'd like to go that way

This comment has been minimized.

@lephilousophe

lephilousophe Apr 25, 2021
Member

OK, so AmigaOS path handling is really a mystry for me. 😄
I can try to do it later in the day or tomorrow. Is it OK for you if I push to your branch?

This comment has been minimized.

@raziel-

raziel- Apr 25, 2021
Author Contributor

Sure.
But take your time.
I want be able to change or test anything before the following weekend.

Thank you for doing this, very much appreciated :-)

This comment has been minimized.

@raziel-

raziel- Apr 25, 2021
Author Contributor

@lephilousophe

Before you invest anymore of your precious coding time into this...
(I just had a lightbulb moment)
...what if i simply change _amigaospath to install/?

Would that satisfy buildbot?

As I understand it, it would install (everything that is processed in amigaos.mk) into a subdirectory called install within the compilation dir?

I could very well live with that.
I'd still need to revert most of the script changes, slightly adapt amigaos.mk and test it, but that would be the most easiest solution, imho.
And you could work on much more awesome stuff...

This comment has been minimized.

@lephilousophe

lephilousophe Apr 26, 2021
Member

using install/ path would work on Buildbot.
I don't think you have to adapt anything to this as you append the / in the path.
Anyway, I don't really like the hardcoded paths for amiga, morphos and win32 which are not configurable.
BTW, I hadn't time to work on this today and next days will be more complicated...

This comment has been minimized.

@raziel-

raziel- Apr 26, 2021
Author Contributor

No problem.
Take your time.

I don't know how intrusive your proposed changes would be and how much adaptation work I'd need to add to the scripts after it was merged, so I'd rather delay my changes until I know.

What I still like to add is a check for the trailing slash (that may or may not come from _amigaospath) and deal with it in all three scripts (amigaos.mk and the two .rexx ones), so that it gets ignored, if it's there.
I have no idea how to do that in the .mk file, though.

@raziel- raziel- changed the title AMIGAOS: Cleanup DO NOT MERGE (YET) - AMIGAOS: Cleanup Apr 25, 2021
raziel- added 3 commits May 1, 2021
@raziel- raziel- changed the title DO NOT MERGE (YET) - AMIGAOS: Cleanup AMIGAOS: Cleanup May 1, 2021
raziel- added 5 commits May 1, 2021
@raziel-
Copy link
Contributor Author

@raziel- raziel- commented May 1, 2021

@lephilousophe

Would that work?
I probably broke something violantly again

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented May 1, 2021

I am looking at it today, didn't have time this week.

@raziel-
Copy link
Contributor Author

@raziel- raziel- commented May 1, 2021

No rush whatsoever :-)
Thank you

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented May 1, 2021

Just push 2 patches:

  • one minor to strip slashes when copying .info file
  • one to fetch sobjs at various places including on toolchain path when cross compiling.
    This last patch was tested by you several weeks ago.

When you are OK with this changes, I can push to Buildbot the needed changes to have correct package building with new path and we will be able to merge this PR.

@raziel-
Copy link
Contributor Author

@raziel- raziel- commented May 1, 2021

Yes, both of them tested and working few weeks ago.
No objections here

lephilousophe added a commit to scummvm/dockerized-bb that referenced this pull request May 1, 2021
@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented May 1, 2021

Merging now as I updated the Buildbot.

@lephilousophe lephilousophe merged commit 7c9a1fa into scummvm:master May 1, 2021
3 checks passed
3 checks passed
@codacy-production
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@deepcode-ci-bot
deepcode-ci-bot Well done, no issues found!
Details
@raziel- raziel- deleted the raziel-:patch-3 branch May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants