Skip to content

Docker fix #90

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Docker fix #90

wants to merge 13 commits into from

Conversation

wardjm
Copy link

@wardjm wardjm commented May 13, 2025

Sorry about taking this on so suddenly. I know you said someone else might be working it. This is all the initial changes required to build and run via docker.

@wardjm
Copy link
Author

wardjm commented May 13, 2025

I should also comment to make sure to test your normal build process. I think everything was done in parallel with *_docker files (since the paths change), but may have accidentally messed something up unwittingly.

network_mode: "host"
ports:
- "53:53" # This breaks DNS, how to ensure localhost doesn't already use 53?
- "65532:65535" # I don't think we use this
Copy link
Owner

@GitHubProUser67 GitHubProUser67 May 13, 2025

Choose a reason for hiding this comment

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

Tada! We do! 65535 is (for now) used for the auto-IP negatiation, but I think... I will change it to be under 65532 anyway (the other port is trojan sensitive).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! So it should be exposed for MitM? That comment reflected that only one of them can bind on that address/port pair, but all of the vcproj files listed it. So it just cannot be true that they all listen on the same port on the same computer.

Copy link
Owner

@GitHubProUser67 GitHubProUser67 May 15, 2025

Choose a reason for hiding this comment

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

You are welcome!

Yes it should, but it is not an obligation.

This can be used by any server.

There is a lock system in place to make sure the port is not used twice at the same time.

And the listener will not remain on all the time, it only open the port and listen when needed.

@Jump-Suit
Copy link
Contributor

As someone who uses MultiServer components dockerized, the edits to the existing dockerfiles do not satisfy docker image build conditions, as the other project references in the .csprojs are missing entries in the dockerfiles themselves.

Just to note, Right-Clicking in VS the project, and Add Docker Support to the project itself, will automatically adjust the dockerfiles to how they should be for compiling even if the files exist.

@wardjm
Copy link
Author

wardjm commented May 14, 2025

As someone who uses MultiServer components dockerized, the edits to the existing dockerfiles do not satisfy docker image build conditions, as the other project references in the .csprojs are missing entries in the dockerfiles themselves.

Just to note, Right-Clicking in VS the project, and Add Docker Support to the project itself, will automatically adjust the dockerfiles to how they should be for compiling even if the files exist.

Thanks! I'd like to get this settled. Is it possible to say specifically what the problem is? Everything builds and runs, so I assume we are just missing a component or two. The entire source tree is copied over to the build images, so they don't have to be listed individually to be built there. Certainly this pull is way closer to correct than what currently exists.

@GitHubProUser67
Copy link
Owner

GitHubProUser67 commented May 14, 2025

I think it now works without doing other project references because since, there is the deps fixer that modified the program startup net config file to take into account costura fody.

Is it possible to test it in a docker linux system?

@Jump-Suit
Copy link
Contributor

I've built my Docker images directly on my backend via Portainer, if the dependencies aren't included in the dockerfiles, they won't be found and will be considered missing upon attempt to compile.

@GitHubProUser67
Copy link
Owner

Can you indicate how to do it?

I think this is worth the inclusion for wardjm but if you know how to do it and where to do it, it can save time I guess.

@GitHubProUser67 GitHubProUser67 force-pushed the main branch 3 times, most recently from f0fea5f to 2b55c09 Compare May 18, 2025 08:53
@wardjm
Copy link
Author

wardjm commented May 19, 2025

I guess we are sort of at an impasse here. Jump claims all of the Docker stuff is broken but if you right-click add docker support it fixes them. But he hasn't committed those fixes for some reason. I claim the current proposed docker compose solution builds everything (listed) correctly. This is verifiable by running the commands in the README. Jump is refusing to state what doesn't build for him. I suspect everything listed builds (but we are missing some components?), but I was trying to go off of what was in the docker files that existed since I hadn't run the programs before. I'm new to the project and don't want to step on anyone's toes, but I need some more specific information if I can fix the problem. This is a docker compose solution as was already in the source code (but it was outdated referencing folders that don't exist anymore). I'm not sure about Portainer as I haven't used it.

@GitHubProUser67
Copy link
Owner

I tested your Pull Request, it does work very well on my wsl docker.

I do agree we need more context if one specific thing we don't know requires fixing.

I will reach out to Jump to see if he can give us the full context.

@GitHubProUser67
Copy link
Owner

So I had some response regarding how to update the dockerfiles. Can you try following these steps to see if it works for you?

J0uOiuJ
TvVOiwU
XZhwyiD

@wardjm
Copy link
Author

wardjm commented Jun 20, 2025

I believe this is updated to work with the Visual Studio changes now. I tested on my end and it seems to work. I only have one game that can connect though.

@GitHubProUser67
Copy link
Owner

Ok, this sounds good, in the next big update there is some of your PR integrated. What game did you tested on it?

@wardjm
Copy link
Author

wardjm commented Jun 20, 2025

I tested it using Riding Club Championships. It can't create an account (and hence can't log in), but I see all the messages going through. I assume the account creation stuff is a separate issue.

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.

3 participants