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

Autoexec fixes #3007

Merged
merged 3 commits into from Oct 21, 2023
Merged

Autoexec fixes #3007

merged 3 commits into from Oct 21, 2023

Conversation

FeralChild64
Copy link
Collaborator

@FeralChild64 FeralChild64 commented Oct 15, 2023

Description

  • restored behavior of dosbox FOOBAR.EXE automounting current directory as drive C:
  • restored behavior of skipping [autoexec] sections if directory, file, or boot image was provided on the command line, and autoexec_section from [dosbox] has value different from join
  • emulator won't try to automount Z: drive from a user provided directory
  • all the mount commands generated are now preceded by unmount attempts
  • commented out the SHELL: Redirecting output to ... log, it is printed out much too often, also when executing some autoexecs

Note: The Z:\AUTOEXEC.BAT generator tries to be smart and friendly - but IMHO it often works in a way which is strange and counterintuitive:

  • imagine we fave FOOBAR.BAT in the current directory; dosbox FOOBAR.BAT will generate CALL FOOBAR.BAT, but dosbox FOOBAR or dosbox "FOOBAR.BAT /switch=y" won't prefix the batch file invocation with CALL
  • one of the conditions to determine whether AUTOEXEC.BAT should get an exit command at the end is the startup verbosity parameter, and it is the value auto that is handled differently
  • if we have a DOOM.BAT file and DOOM directory, dosbox DOOM will try to mount the directory, but won't launch the game

I'm not using this functionality, I don't know what was the rationale behind them, so it is hard for me to propose a solution that is logical and won't upset users.


Related issues

#2987

Manual testing

I recomment to uncomment #define DEBUG_AUTOEXEC in src/shell/autoexec.cpp and recompile before playing with various options - this will print out the generated Z:\AUTOEXEC.BAT to the logs, making it easier to see what is actually happening.

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

@FeralChild64 FeralChild64 added the regression We broke something 😊 label Oct 15, 2023
@FeralChild64 FeralChild64 self-assigned this Oct 15, 2023
@FeralChild64 FeralChild64 changed the title [DRAFT] Autoexec fixes Autoexec fixes Oct 15, 2023
@FeralChild64 FeralChild64 marked this pull request as ready for review October 15, 2023 06:51
@johnnovak
Copy link
Member

johnnovak commented Oct 15, 2023

Thanks, @FeralChild64.

I'm not using this functionality, I don't know what was the rationale behind them, so it is hard for me to propose a solution that is logical and won't upset users.

I think it's probably good overall that we restored this functionality, but if it's ultimately something no one in the dev team cares about and it's weird, counterintuitive, obscure, complicated, and edge case-y, maybe it's chopping block time... 😄

We'll see how this goes and how much trouble it causes down the road.

src/shell/autoexec.cpp Outdated Show resolved Hide resolved
Copy link
Member

@johnnovak johnnovak left a comment

Choose a reason for hiding this comment

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

Code looks fine @FeralChild64 and I have really low interest in this feature so I trust your manual testing 😄

@weirddan455
Copy link
Collaborator

Did some testing.

cd /home/daniel/dos/DOOM_SE
/home/daniel/code/dosbox-staging/build/debug/dosbox DOOM.EXE

This works 👍

cd /home/daniel/code/dosbox-staging
build/debug/dosbox /home/daniel/dos/DOOM_SE/DOOM.EXE

This does not work and is a regression from 0.80.1

restored behavior of dosbox FOOBAR.EXE automounting current directory as drive C:

I believe this is the problem. Behavior should not be to mount current directory but instead mount the directory the .exe is located in.

I'm going to stay out of the argument of whether we should keep this feature in. I think @kcgen made some good points about why we should have this.

@FeralChild64
Copy link
Collaborator Author

cd /home/daniel/code/dosbox-staging
build/debug/dosbox /home/daniel/dos/DOOM_SE/DOOM.EXE

This does not work and is a regression from 0.80.1

restored behavior of dosbox FOOBAR.EXE automounting current directory as drive C:

I believe this is the problem. Behavior should not be to mount current directory but instead mount the directory the .exe is located in.

OK, I'll investigate it. Switching back to draft for now.

@FeralChild64 FeralChild64 linked an issue Oct 15, 2023 that may be closed by this pull request
3 tasks
@FeralChild64 FeralChild64 marked this pull request as draft October 15, 2023 12:47
src/shell/autoexec.cpp Outdated Show resolved Hide resolved
src/shell/autoexec.cpp Outdated Show resolved Hide resolved
src/shell/autoexec.cpp Outdated Show resolved Hide resolved
src/shell/autoexec.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Show resolved Hide resolved
src/shell/autoexec.cpp Outdated Show resolved Hide resolved
@kcgen kcgen self-requested a review October 15, 2023 19:13
@FeralChild64 FeralChild64 force-pushed the fc/autoexec-fix-3 branch 3 times, most recently from b51832d to 831ca38 Compare October 21, 2023 08:24
@FeralChild64 FeralChild64 marked this pull request as ready for review October 21, 2023 08:47
@FeralChild64
Copy link
Collaborator Author

@weirddan455 Should be fixed now.
@kcgen You can continue the review.

Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

Working great, @FeralChild64.
Executables with relative paths, absolute paths, and those without any path (local files) all mount and launch without issue. 👍

Baseline tests (-c, -conf, and without args) are running good too.

@kcgen
Copy link
Member

kcgen commented Oct 21, 2023

I think everyone's reviewed the code, and with the regression fixed, it's time to merge.

@kcgen kcgen merged commit 46f18bd into main Oct 21, 2023
50 checks passed
@es20490446e
Copy link

Thanks for the fix, number one 🥇

@FeralChild64 FeralChild64 deleted the fc/autoexec-fix-3 branch October 22, 2023 11:35
@johnnovak johnnovak added the enhancement New feature or enhancement of existing features label Nov 13, 2023
@johnnovak johnnovak added the DOS Issues related to DOS integration or DOS commands label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DOS Issues related to DOS integration or DOS commands enhancement New feature or enhancement of existing features regression We broke something 😊
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

dosbox FOOBAR.EXE no longer mounts and launches the executable
5 participants