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

[Feature] Add devcontainer support #4294

Merged
merged 3 commits into from
May 9, 2024

Conversation

xackery
Copy link
Contributor

@xackery xackery commented May 2, 2024

Description

This adds devcontainer support

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Testing

I noted this as breaking change since folks who use .vscode previous may find this impacts their configuration

if PR gets accepted, I'll add a documentation page with this info

Features and testing:

  • Testing included doing steps noted in this info.
  • Tested autocomplete/intellisense works within dev container
  • Tested precompiler flags being enabled via c_cpp_properties.json defines, we may need to add some more for full cohesion later.
  • Tested running make docker-cmake and make docker-build outside of dev container for folks who just want to quickly build binaries.
  • Tested include paths via c_cpp_properties.json
  • Tested running with gdb using the built in vscode debug operations (breakpoints, stacktrace, etc all work within vscode)
  • Tested running valgrind via the make valgrind-zone and make valgrind-world commands
  • Tested make depends, this uses a depdency graph to visualize eqemu includes, this is an intense operation however, especially on zone
  • Tested backups using make backup
  • Tested cpu profiling using make cpu-zone, then generating reports using make pprof-zone
  • Tested connecting to db locally using the port forwarding methods (localhost)
  • Tested connecting to db remotely using LAN IP ports
  • Need to test login server configuration still
  • We may need to tweak the ninja/ccache building flags, I did not heavily optimize things and especially within a dev container memory cost is pretty high, I imagine spire's build pipeline can be tapped into to optimize this.
  • Documentation likely needs additional steps for WAN-access instructions
  • Documentation has references to jamfest, when I formally make documentation I'll be sure to remove those references.

Clients tested: RoF2, but it's a service side change

Checklist

  • I have tested my changes
  • I have performed a self-review of my code. Ensuring variables, functions and methods are named in a human-readable way, comments are added only where naming of variables, functions and methods can't give enough context.
  • I have made corresponding changes to the documentation (if applicable, if not delete this line)
  • I own the changes of my code and take responsibility for the potential issues that occur

Copy link
Member

@Akkadius Akkadius left a comment

Choose a reason for hiding this comment

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

I'm fine with this going in for the most part, but I have a few things -

This is an opt-in development experience for those who want to develop with VSCode and the devcontainer model - anyone who wants to use that I think it is great someone is procuring that experience and making it better for those who want it. Devcontainer spec has been picking up utilization across different audiences.

Assets that pertain to this experience should nest themselves under .devcontainer. They keep the assets scoped to the .devcontainer experience, not to confuse with what's generally maintained in the installer outside of the base repository.

Similarly with Makefile have that also nested under .devcontainer folder. Your entrypoint for your devcontainer configuration can symlink it to the root of the devcontainer experience.

To contrast, I and others have been developing in akk-stack for ~5 years now and the entire experience is decoupled from the eqemu/server repository and is also entirely an opt-in experience.

Keeping these things decoupled helps reduce confusion and inferring that there's one way to do development and folks can opt-in to patterns and experiences they prefer.

@xackery
Copy link
Contributor Author

xackery commented May 5, 2024

This is an opt-in development experience for those who want to develop with VSCode and the devcontainer model -

Definitely, in the documentation I'll be sure to note this is optional, and give light to akk-stack instruction as well.

Assets that pertain to this experience should nest themselves under .devcontainer.

Latest commit should address this.

Similarly with Makefile have that also nested under .devcontainer folder.

Done. I made it where you can run make from the devcontainer, or symlink it, both should work.

@Akkadius Akkadius merged commit d6f1bba into EQEmu:master May 9, 2024
1 check passed
@Akkadius Akkadius mentioned this pull request May 9, 2024
catapultam-habeo pushed a commit to The-Heroes-Journey-EQEMU/Server that referenced this pull request Jul 14, 2024
* Add devcontainer support

* Rename default values for eqemu_config

* Move devcontainer files to devcontainer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants