search

Rogier-5 / minetest-mapper-cpp

Generates maps of minetest and freeminer worlds
Other
9 stars 4 forks source link

Build on Windows using MSVC #10

Closed adrido closed 8 years ago

Rogier-5 commented 8 years ago

Looks good. Some remarks:

I haven't looked at the version stuff yet, in particular the batch file...

adrido commented 8 years ago

MSVC/minetestmapper.vcxproj.filters: there are a number of German strings in there... Could they be in english ? And will a non-german-localized user be confronted with them (and maybe baffled :-) ?

AFAIK they get autotranslated again. I have to do further tests to confirm or maybe turn it off.

MSVC/versioninfo.h is generated when building, or is it ? So presumably it shouldn't be in git ?

Uhm, yes of course you're right.

github says MSVC/minetestmapper.rc is binary, while the contents are text. Was it checked in in binary mode ? Or should it be binary ?

Some years ago I have read that the rc file should always used as binary files because it easy gets broken if git converts line-endings or merges some changes. Sorry, but I cannot find the source any more, maybe this behaviour has changed some day.

  • For the patch open -> fopen, could you please mention in the commit message that this was changed for the windows port.
  • Same for getopt and dirent. For those two, please also document why you think including copies of foreign code is a good idea (i.e. your earlier explanation). In a few years, a developer may be wondering why on earth that was done... The best place may be in the respective README.txt files.

I'm not expert in using Git. So please don't be annoyed if I ask you if there a simple way to change already committed and pushed commits?

Rogier-5 commented 8 years ago

github says MSVC/minetestmapper.rc is binary, while the contents are text. Was it checked in in binary mode ? Or should it be binary ?

Some years ago I have read that the rc file should always used as binary files because it easy gets broken if git converts line-endings

I got it: the file is not UTF-8, but UTF-16. So git rightfully considers it as binary (from an UTF-8 point of view). Preferably git would be UTF-16 aware, so that it could checkout such files as UTF-8 on linux and as UTF-16 on windows (99% of tools people would want to use on linux are UTF-8). But I suspect git won't be for the forseeable future...

If possible, I'd prefer it to be UTF-8 format. You might try the suggestion at the bottom of this thread, or the first suggestion here. If you like, I'll be happy to provide you with a converted file for testing.

BTW: I just saw that minetest also has an UTF-8 .rc file. Obviously, I don't know how they achieved it...

or merges some changes.

That is indeed a real danger - for all commits. Sometimes, git merges stuff incorrectly. For text files that are in practise basically not suitable to be edited by humans, this can be a problem... Such files should probably be marked 'binary' or 'merge=binary' - see below.

Distinct from the encoding used to store the file (must be UTF-8 for text files, or if necessary UTF-16 - anything else is nonportable), is whether git treats the files as binary (no automatic line-ending conversion, no automatic merging of contents: manual merging required) or text (conversion and merging is done automatically). To force treating msvc UTF-8 project files as binary, add a line <filename-or-pattern> binary diff in .gitattributes (if I'm correct). This should both disable conversion and automatic merges, and still allow diff to work. <filename-or-pattern> merge=binary would just disable automatic merges.

  • For the patch open -> fopen, could you please mention in the commit message that this was changed for the windows port.
  • Same for getopt and dirent. For those two, please also document why you think including copies of foreign code is a good idea (i.e. your earlier explanation). In a few years, a developer may be wondering why on earth that was done... The best place may be in the respective README.txt files.

I'm not expert in using Git. So please don't be annoyed if I ask you if there a simple way to change already committed and pushed commits?

Not annoyed at all.

Are you referring to the changing of a committed commit locally, or to the pushing of the changed branch afterwards ?

In the first case:

For changing a commit that is at the top of the branch, use git commit --amend.

Else I'd recommend to make the changes on the top of your existing branch, commit them using git commit --fixup <commit-to-be-changed> and then do an interactive rebase: git rebase -i --autosquash <root-of-tree-to-be-rebased> (see the manual; basically with git rebase -i you can reorder commits, edit commit messages, combine commits, modify commits themselves, etc. on a series of commits. Without --autosquash, the --fixup commits are not automatically reordered. If you didn't commit using --fixup, you'll have to reorder the commits manually).

Sometimes I also create a new branch at the commit to be changed, then edit, git commit --amend (which also allows editing the message), and then git cherry-pick the other commits from the old branch (or git rebase --onto <newbranch> <parent-of-first-commit-to-be-moved>). That is basically the same as what git rebase -i does, but it does it in a more automated fashion...

In order to push the reworked tree from the command line, you add the '-f' option e.g.: git push -f origin windows-2 or: git push -f <name-of-remote> <localbranchname>:<remotebranchname>

The '-f' forces the rewriting of remote history, which is normally (i.e. for official branches) strongly discouraged.

After pushing, if you used the remote branch name of the pull request, github will automatically update the pull request.

I assume that the windows command-line client has the same syntax as on linux... If you are using a graphical client, I suppose the terms will be the same: fixup, rebase (interactive), amend and push + force. If you're using a graphical client, forced push might have been disabled. You might have to the push from the command-line...

are you using a graphical client ? Only ?

And BTW: git add -p is a nice way to do partial checkins and commits.

I hope this answers your question. If not, please don't hesitate to ask for clarification.

adrido commented 8 years ago

Thanks allot, that helped very much. I DuckDuckgoed much, but I only found only diffrent forum threads answerd by people which had less experience than me :confused:

the --fixup and --autosqaush where the keywords I had missed. Thanks again :smile:

are you using a graphical client ? Only ?

^^ Yes and no. Yes, I'm using a graphical client, (not the Github Client, it lacks on professionallity and features, and it does not show which commands it executes. I use SourceTree which is more professional and shows the git commands it executes.) but I also use the console for automatism or do some stuff that SourceTree does not support. Im not such a noob, although I have to say that I prefer a well programmed GUI client over a console application.

I could successfully convert the encoding of minetestmapper.rc to Utf-8 .Another good thing is, the resource compiler got improved, so it is also able to compile with linux line-endings. (At least with (VS2013 update 4 and VS2015 Enterprise update 1) Its now checked in as text file.

I translated the phrases in vcxproj and .filters files.

I think it should be able to merge now.

Rogier-5 commented 8 years ago

Hi Adrido,

Sorry to have kept you waiting some time. I have been quite busy.

I pushed a new branch to my repository with your patches and a few changes that I made to them.

Summary:

If you could test this version, then I can fix any bugs in the batch file (or you can fix them of course, if you like), so that I can finally merge this PR.

Kind regards.

adrido commented 8 years ago

Ill take a look to it today or tomorrow evening.

Am 17.05.2016 um 16:59 schrieb Rogier-5:

Hi Adrido,

Sorry to have kept you waiting some time. I have been quite busy.

I pushed a new branch to my repository with your patches and a few changes that I made to them.

Summary:

*

I replaced the batch file with one I wrote. The other one was
quite complex, did more than necessary, and I prefer it to use the
existing version file (renamed to base-version) and
cmake_config.h.in (renamed to build_config.h.in). The file also
lacked a license, but if it weren't for my other objections, I
could have contacted the author of course.

As, regrettably, I don't have a Windows machine at my disposal,
the batch file is utterly untested...

*

I also made some changes to the minetestmapper.rc file, so that it
now uses more symbolic constants. I hope this is not inconvenient,
and that it does not cause MSVC to fail ?

*

I integrated the building instructions into the existing
build-instructions.rst

*

And I rebased your work on my latest tree. The tree now includes
character set conversion (disabled at the moment for MSVC) and
postgresql support (also disabled).

If you could test this version, then I can fix any bugs in the batch file (or you can fix them of course, if you like), so that I can finally merge this PR.

Kind regards.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/Rogier-5/minetest-mapper-cpp/pull/10#issuecomment-219745139