This fixes the issues described in #1532

Since you have 4 pretty much distinct changes, it is probably best to make 4 different pullrequests for it.

Considering that 1) is a relatively small change (< 15 lines), 2) it all goes towards the same feature (enabling building project for Visual Studio 2015), and 3) for everyone's sanity; I would strongly prefer to keep this as a single pull request.

The commits have been squashed, since all comments have been addressed.

Thanks, this looks much better, but as was pointed out earlier, each change which does something separate should go in a separate commit (for example, I'd be happy to apply 2 of the changes above right away, but the rest will need explicit confirmation of @stinos). I even looked a way to do git cherry-pick -p (which shamingly doesn't exist), but there's none which allows to preserve the original author.

@pfalcon I fail to see which changes in this PR are part of something separate. All these changes enable building in Visual Studio. Are you suggesting one PR per file? Per line?

All these changes enable building in Visual Studio.

Yes, and all commits in this repo can be squashed to one with commit message "MicroPython". And yet we do it differently, and there're even formal guidelines how to do it (so, it's not about your patch - it's about every patch).

Are you suggesting one PR per file? Per line?

Per separate change, as guidelines explain. For example, I'd to 3 changes here: one to makeversionhdr.py
to let it work properly with backslash paths (and commit message should say that, not some generic "in the name of"), another one to call _set_output_format() - note that it has nothing to do with building, so the commit message is plain wrong (I also could ask if you really confirmed it doesn't work for mingw, but I'm skipping that, as I'd really like to merge it). These 2 commits I'd be happy to merge right away, because they are pretty clear even for me, who doesn't run MSVC. The rest will need to wait for ack from @stinos as the MSVC build system maintainer.

Sorry if it seems so hard - it doesn't have to be, and guidelines were written to avoid obvious problems. So I'm wondering just the same, why it's so hard to follow pretty common-sense rule - don't throw your veggies mixed up into a fridge, keep each type separate in its own box, for easy access and maintenance.

call _set_output_format() - note that it has nothing to do with building, so the commit message is plain wrong

Actually, due to MSVC 14.0 breaking change (as explained in the link which is part of the comments above the line change) building does fail without that #ifdef which, by the logic implied, should not affect mingw.

I understand that there are rules and guidelines for a reason, but I also believe that there is an incremental improvement between project build failing and project build working. I also presumed that the windows port, being labeled as experimental, would not be subject to such strict (almost dogmatic?) review process when none of the changes affect other ports.

The bottom line is that I wanted a binary for different Windows flavors; the Windows project was completely broken and I found a way to fix it for myself which required 1) the backslash fix 2) the output format flag and 3) the binary output folder structure. I thought that I would give back to the project by sharing what the fix was, adjusting for the feedback, and adhering to the project guidelines to the best of my abilities.

Being pretty new to Git, and with the problem I was encountering solved for me, I'm afraid that I don't have the time to open 3 separate pull requests and, frankly, that seems quite unreasonable for a ~10 line change.

and commit message should say that, not some generic "in the name of"

That is good feedback, more coherent and verbose commit messages is something I should be more diligent about.

@omtinez it would help if you could separate the changes into 2 or 3 separate commits, for each logical thing (esp separating out the change to py/makeversionhdr.py). Then we can merge them individually and you don't need 3 separate PRs.

Of course we could just make the changes ourselves, but it's nice to have your name as the author of the commit in the history of the git log (and I hope that's something you want!).

@dpgeorge I think that I was able to break it out into the discrete commits, let me know if my branch now contains what you requested.

@pfalcon do you think that the discrete commits adhere to the project guidelines better or would you still prefer independent pull requests? Any comments/thoughts about the commit messages?

@stinos what do you think about the changes in the MSVC project? As stated in the commit message, having a different binary output path for different project configurations is the only way to build for different flavors without overwriting the output. Additionally, the produced binaries do not get dumped in the same folder as code and it is better aligned with the default project configuration in Visual Studio.

@pfalcon do you think that the discrete commits adhere to the project guidelines better or would you still prefer independent pull requests? Any comments/thoughts about the commit messages?

No independent pull requests are needed (in this case). I've merged 17c649d and 3510499 , indeed adjusted commit messages a bit, hope you're ok with that. Thanks for your patches and patience! You may want to rebase your branch and re-push it (with --force switch), so only remaining patches were shown here.

@stinos : Please ack that the rest is ok to go. As a minor nitpick, there's:

+    <OutDir>$(PyOutDir)$(Configuration)\$(Platform)\</OutDir>
     <IntDir>$(PyBuildDir)$(Configuration)$(Platform)\</IntDir>

(see backslash difference). I'm personally ok with it, just want to be sure it doesn't break anything ;-)

being labeled as experimental

good point, maybe it's time to remove that label, after all the port works fine, passes almost all tests and imo has enough features to be used as a small/fast python implementation. Also the README explicitely mentions VS2013 so after these changes get merged that should change as well

such strict (almost dogmatic?) review process when none of the changes affect other ports

experimental or not, being strict (as in, basically deciding for each single line changed whether it is ok for the project in the longer run) is probably the only way to maintain a public open source project without turning it into a mess. Also other ports not being affected is orthogonal to other users/developers not being affected so that point is moot.

do you think that the discrete commits adhere to the project guidelines better

much better, but still some minor things left: commit messages should be prefixed with the directory (I never used this before contributing to uPy but it grew onto me now and it actually makes a lot of sense) and should, if possible, also explain why a change was made, next to what was changed (the former documents the latter so PR reviewers know what the commit is about, the latter does not always need to be stated again since the diff does that already), so your first message could for example be something like

py: Replace forward slashes with OS independent os.path calls

This fixes errors in makeversionhdr.py when using paths paths
with backward slashes on Windows.

Also note that github deals with such format nicely when creating a PR: first line is the title of thr PR, other lines go in the PR description.

Regarding the individual changes:

• _set_output_format change is ok, as stated in the VS2015 changelog you linked to
• project update is ok, as it enables build using VS2015 and appears to not affect VS2013, and older versions lack the C99 support to build uPy anyway so they don't matter

Anyway, just saw that pfalcon merged the above already - including proposed changes - while I was writing this; I've some things to take care of now and I'll be back later with comments on the output dir change, thanks for your efforts so far.

Few comments from me too:

building does fail without that #ifdef which, by the logic implied, should not affect mingw.

That's why review process - you see, my implied logic doesn't match your implied logic, if you said it clearly (preferrably in commit message), there wouldn't be more questions.

The bottom line is that I wanted a binary for different Windows flavors; the Windows project was completely broken

It was not! We build Windows as part of our Continuous Integartion loop, and the badge on https://github.com/micropython/micropython in README is always (well, 99.9%) green.

none of the changes affect other ports.

makeversionhdr.py change does affect.

for a ~10 line change.

It's a small change because that stuff was already polished pretty much, the more we're interested to understand why more changes are still required. Because otherwise it's easy to go over top of the hill and actually deteriorate it.

I'm afraid that I don't have the time to open 3 separate pull requests

We don't do all this to for bureaucracy's sake - we hate it very much (speaking for myself, I contribute patches to various projects, and sometimes less than satisfied with the process, so trying my best to do it better for uPy). So, no 3 pull requests are required (3 commits - yes). But would you be able in 6 months to answer why each of the changes was made in the original patch you submitted with the original description? Sorry, I doubt that (and 6 months is optimistic figure, most real-world multitasking people won't be able in 1 month). Now imagine that in 5 month comes some guy, saying "project completely broken" and pointing at your commit. You may imagine a possible outcome - the complete commit may get reverted, because indeed, it's questionable. With our process, questions resolved first, and whoever may come later saying that it's completely broken, will get patient hints that it's not, and more homework is required to actually isolate issue(s) and find teh best way to fix them without affecting what's already worked for people so far (and otherwise reshuffling stuff). Hope all that is actually something to appreciate.

Thanks @omtinez for splitting it into 3 commits.

For your interest, and others following the discussion, here is a random selection of commits with detailed messages, sometimes the message is bigger than the commit: bedab23 dfa915a d80174d 01d6491

The project is at a stage now where lots of people use and rely on it and changing some code can have an unforeseen impact (and changing code related to the build process is one area that can easily cause problems). To mitigate such problems we need to document the development well, and that comes through proper git commit messages.

Please ack that the rest is ok to go.

• project change: ok for me, though ideally it would be accompanied by a modification of the port's README so it states the new functionality
• output directory change: not ok in it's current state after some investigation
  • it's different from what all (?) the other ports use
  • as such it also breaks running tests with default settings (1)
  • not a fan of $(Configuration)$(Platform) format, there are enough directories already, without the slash is better imo
  • build is easy enough to customise for that or any other output directory the user wants (2)
  • my mistake, sorry for that, but the change should be made in env.props not common.props - reason being the first is meant to actually declare all directories and is included by multiple other files

(1) Now you just run python run-tests no matter what platform/configuration used and no matter whether building on unix, windows mingw or windows msvc. To accomodate this change however, either one has to use e.g. set MICROPY_MICROPYTHON=..\windows\Debug\Win32\micropython.exe first (and hope that matches the platform/config last built) or the default path used in run-tests has to be changed (but then we'd have to fight over what that default should be.. and in any case that would break test running for mingw builds). None of these are nice solutions, especially given they are solutions to a problem which didn't exist in the first place. @omtinez out of interest: which tests fail using the VS2015 builds?

(2) Place user.props in msvc directory and adjust any build setting to taste, it's ignored by git btw

However after checking the current situation I'm also not too happy with the windows dir being littered with map/pdb/whatever files and I agree it might be handy to have different exes simultaneously. Also the build output now goes to windows/msvc/build while mingw build output goes to windows/build (which is consistent with other ports).

Long story short: I'd fix this using a 'best of both worlds approach' putting msvc executables in windows/build as well, using $(Configuration)$(Platform) subdirectory, and object files in again a subdirectory of that, and add a post-build step which copies the executable to the windows directory no matter which flavor is built. This seems the only way to address as much as possible of the considerations raised by all parties. Please comment. @omtinez if this suits you but you don't have time let me know and I'll be on it.

You may want to rebase your branch and re-push it

Done.

the README explicitely mentions VS2013 so after these changes get merged that should change as well

I agree with this, I did not notice that.

my implied logic doesn't match your implied logic, if you said it clearly (preferrably in commit message), there wouldn't be more questions.

This was miscommunication on my part. What I meant by implied logic was the the #elif can't, by the logic of the #ifdef, affect mingw.

the Windows project was completely broken

It was not! We build Windows as part of our Continuous Integartion loop

I am very surprised by this, considering the state of the build process when I first attempted it on VS2015.

But would you be able in 6 months to answer why each of the changes was made in the original patch you submitted with the original description? Sorry, I doubt that

Fair enough, I never questioned that. The single-PR and single-commit is a fairly common way of managing external contributions. Realistically, not many times there is a need to have granularity to the individual commit level. Obviously, it's a matter of preference.

To mitigate such problems we need to document the development well, and that comes through proper git commit messages.

See above. While I personally agree with more granular information, most other projects prefer single-commit and single-PR. That's my failure for not adhering to the stated project guidelines and making assumptions instead.

@stinos : the project configuration is more about standards than about personal preference. Adhering to the platform standards makes it much easier for new developers/users to understand it from the get-go. Maybe not as important as some might tell you, but certain tools depend on (hope might be a better word?) standard binary output directories. I, for one, have a number of scripts that call MSBUILD and fish the appropriate binaries out of the expected output folder.

I can move the changes from common.props to env.props, but before I do that I will take a look at the situation with the tests and report back.

Thank you all for your patience with this, I'm happy to be able to contribute to this project.

standards

There isn't really a standard here, just a default used by one IDE on one particular platform. If there's one standard way to figure out where any build system puts it's output files then the standard is looking at the build output or inspecting the build files. Users, especially developers, or scripts should know better than to expect things, they better be sure. As an anecdotical exercise I went through all projects which I currently have on my pc (I'll name some known ones for illustration: CPython, Boost, LLVM, CLang, NuGet) , and none of them uses the default. Which basically tells us each project uses it's own standards - in this case: build in build subdirectory and put executable in port directory.

Users, especially developers, or scripts should know better than to expect things, they better be sure

In theory. In practice, any project worth its salt inevitably evolves into having a somewhat complex build system and making assumptions about the outputs is the only sensible way to tackle some issues. I know of at least one project where post-build test running breaks after the binary output location is changed :-) What matters to me is that I can call MSBUILD with certain parameters and expect the output to be in a certain location which will not be overwritten by subsequent MSBUILD calls with different parameters. If it's in the default location, then it saves me the trouble of having to look at the actual project files to see what goes where.

CPython, Boost, LLVM, CLang, NuGet

Those are all aimed to be platform-independent; I'd say that, in my anecdotal experience, I have rarely seen Windows-only projects or projects that started as Windows-only and don't follow the IDE default. Removing one level of nested folders like you suggest is OK with me but I personally don't find it as clean as the default. I guess my question should be more like is there a good reason not to use the default? In this case, it breaks uniformity with respect to the other ports, which qualifies as a solid reason in my book.

What matters to me is that I can call MSBUILD with certain parameters and expect the output to be in a certain location which will not be overwritten by subsequent MSBUILD calls with different parameters. If it's in the default location, then it saves me the trouble of having to look at the actual project files to see what goes where.

Fair enough, the main thing I have against the default is that it puts one directory per configuration right in the source, that's almost on the same level as just putting all output files in the source dir. I care less whether it has extra nested subdirs so just see if putting whatever subdir format under the build directory suits you.

Btw the default VS uses here is $(SolutionDir)$(Platform)$(Configuration), so the last two are the reverse of your suggestion?

So as it happens just yesterday I ran into linker errors when switching between build configurations, because all output goes into one single directory. The problem has always been there of course but it went largely unnoticed and only now I happened to need a ton of memory which lead to switching back and forth between x86/x64 builds. @omtinez stinos@fb8939a is the proposed fix which is basically the idea I laid out in the above comments somewhere.

@stinos I am looking over the commit that you linked, please tell me if I got it right:

1. The output directory is structured in the way that you proposed, e.g. windows\DebugWin32*
2. After the build, the resulting .exe is copied into windows\ like with the other ports

This seems to me like the best compromise. It's not adding any more nested directories than strictly necessary, and the output is both where one would expect it to be (+/- the slightly different directory structure) and consistent with the other ports.

e.g. windows\DebugWin32*

actually it's windows\build\DebugWin32, so all build artifacts go under the build directory. That seemed cleaner to me than polluting the windows directory with one subdirectory per build type (now that's 'only' 4, but suppose uPy can be built into a static library by selecting configurations with a different name than it blows up) plus after all that's what the build directory is meant for.

Sounds good to me. Was that the last issue to be addressed by this pull request? Should I close it now?

Not yet: your commit with the upgraded project file hasn't been merged into master yet. If you remove your change to common.props and rebase then @pfalcon can merge this. After which this PR and #1532 can be closed and VS2015 support is fixed.

@stinos: Please take a look, I think that I was able to rebase with the latest changes and removed my edits to common.props

Looks good! Ok for me to merge this.

Merged, thanks guys!