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

dosbox-staging: init at v0.75.0 #85744

Closed
wants to merge 5 commits into from
Closed

dosbox-staging: init at v0.75.0 #85744

wants to merge 5 commits into from

Conversation

JoshuaFern
Copy link
Member

@JoshuaFern JoshuaFern commented Apr 22, 2020

Motivation for this change

dosbox-staging attempts to modernize the DOSBox codebase by using current development practices and tools, fixing issues, and adding features that better support today's systems.

If you review and install this package, please check that your icon/desktop file is working correctly on your DE of choice.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@bbjubjub2494 bbjubjub2494 left a comment

Choose a reason for hiding this comment

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

Tested under NixOS 20.03 and Gnome 3, the .desktop is working perfectly.

The official website wasn't ready until recently.

Co-Authored-By: Louis Bettens <lourkeur@users.noreply.github.com>
Copy link
Member

@bbjubjub2494 bbjubjub2494 left a comment

Choose a reason for hiding this comment

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

  • meta.homepage
    LGTM

@JoshuaFern JoshuaFern changed the title dosbox-staging: init at v0.75.0-rc1 dosbox-staging: init at v0.75.0 May 6, 2020
@JoshuaFern
Copy link
Member Author

Happy release day! The changes between versions is minimal.

I think this is ready to merge @aanderse

@bbjubjub2494
Copy link
Member

No regression re: GNOME3 experience 👍

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Unsure where I fit into this PR... but I hope these comments are more helpful than confusing.

pkgs/misc/emulators/dosbox-staging/default.nix Outdated Show resolved Hide resolved
pkgs/misc/emulators/dosbox-staging/default.nix Outdated Show resolved Hide resolved
pkgs/misc/emulators/dosbox-staging/default.nix Outdated Show resolved Hide resolved
pkgs/misc/emulators/dosbox-staging/default.nix Outdated Show resolved Hide resolved
preConfigure = "./autogen.sh";

preBuild = ''
buildFlagsArray=( "CXXFLAGS=-O3 -DNDEBUG" )
Copy link
Member

Choose a reason for hiding this comment

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

Are these actually required? If so, wouldn't something like buildFlags = [ "CXXFLAGS=-03" ]; be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe -DNDEBUG is required, maybe @dreamer could elaborate why.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had trouble getting buildFlags to play nice with -DNDEBUG so I used CXXFLAGS = "-O3 -DNDEBUG"; instead after I saw a few other packages using this.

Copy link

Choose a reason for hiding this comment

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

Both are required for release builds (in fact more compilation flags might be beneficial, with different sets per architecture, but simple -O3 -DNDEBUG is good enough across the board and good enough for the first release :)

Explanation:

  • NDEBUG is standard POSIX-defined flag, that should be used for all C/C++ projects providing release binaries. GNU doc.
  • -O3 - the codebase is written in a way, that's very heavily affected by compiler optimizations; this level enables some crucial ones (like loop unrolling, switch/loop unrolling, vectorization, predictive commoning, etc)

One more note: dosbox-staging is mixed C/C++ codebase, so please use:

CFLAGS=-O3 -DNDEBUG
CXXFLAGS=-O3 -DNDEBUG

Copy link
Member

Choose a reason for hiding this comment

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

@JoshuaFern I should have been more specific... I'm wondering if these values are needed in the derivation because maybe they are already implied by our build system. I don't know and this area is certainly a weak point in my knowledge. You could dig into our docs, builder code, or ping one of many people who know off the top of their head. Sorry I'm not being more help 😞

Copy link
Member Author

@JoshuaFern JoshuaFern May 8, 2020

Choose a reason for hiding this comment

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

Running the build and searching through the log I'm not seeing -DNDEBUG and it's using -02 by default. I believe we need these manually defined.

Sorry I'm not being more help

You have good suggestions, thanks! :)

@JoshuaFern JoshuaFern requested a review from aanderse May 8, 2020 03:58
@aanderse
Copy link
Member

aanderse commented May 9, 2020

@JoshuaFern good stuff. At this point I recommend you get an automake expert to take a look.

@JoshuaFern
Copy link
Member Author

This PR has gone a bit stale, I'm closing it for now.

@JoshuaFern JoshuaFern closed this Apr 12, 2021
@JoshuaFern JoshuaFern deleted the dosbox-staging branch October 8, 2021 23:28
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.

None yet

5 participants