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

Winbuild: Updated BUILD.WINDOWS.txt to reflect more recent data #2472

Closed
wants to merge 12 commits into from
Closed

Winbuild: Updated BUILD.WINDOWS.txt to reflect more recent data #2472

wants to merge 12 commits into from

Conversation

kdekker
Copy link
Contributor

@kdekker kdekker commented Apr 9, 2018

  1. Use URLs that point to newer versions of SDKs/documentation
  2. Updated text to reflect more recent Visual Studio versions (2015 and 2017)

kdekker added 12 commits Apr 4, 2018
Due to the check in Makefile.vc and MakefileBuild.vc, no make call can be invoked unless a build-type was specified. However, a clean target only existed when a build type was specified. As a result, the clean target was unreachable. Made clean target unconditional.
In modern Windows versions and/or more recent Visual Studio compilers, no setenv command exists. The way to open a CMD prompt with right settings is either done by vcvarsall.bat or VsDevCmd.bat.

Also updated URLs to more recent SDK versions. The SDK is no longer called "platform SDK" but "Windows SDK" (by Microsoft).
The documentation was quite outdated. The setenv command no longer exists and visual studio build prompts got changed. Used Visual Studio 2015/2017 as reference.
@kdekker kdekker mentioned this pull request Apr 9, 2018
@bagder
Copy link
Member

@bagder bagder commented Apr 9, 2018

This PR has 11 irrelevant commits and 1 that you want merged...

@bagder bagder closed this in 73070e8 Apr 9, 2018
@bagder
Copy link
Member

@bagder bagder commented Apr 9, 2018

I cherry-picked the single patch and merged that now. Please also check our template for commit messages, as that makes it even easier for us to receive your changes.

Thanks!

@kdekker
Copy link
Contributor Author

@kdekker kdekker commented Apr 9, 2018

I reverted all on master branch of my own clone + forced (based on the comments of Marcel Raad) a sync with the remote repository. I don't know how to find your template. I'm almost struggling for 3 days how to create a PR. I understand that you have not that much time to explain, but just 'check our template' causes me to spend very much time, on just a simple documentation change. It requires really much courage (and time) to contribute to curl... I'm a little bit disappointed how much.

It would be great if you can provide an URL to the mentioned template.

@bagder
Copy link
Member

@bagder bagder commented Apr 9, 2018

It requires really much courage (and time) to contribute to curl... I'm a little bit disappointed how much.

To me, it seems this time you speak of is mostly you not knowing the primary tool we use: git. I don't know what we can do to make that ride easier... Also, making PRs on github is widely documented on github and elsewhere and also a procedure that is shared by millions of projects so lots of users already know how, plus once you've learned you'll be able to use this knowledge in many other projects.

The preferred commit message style is described in the regular contribute page. See the "Write good commit messages" subtitle.

@@ -12,7 +12,7 @@ Building with Visual C++, prerequisites

The latest Platform SDK can be downloaded freely from:

https://msdn.microsoft.com/en-us/windows/bb980924
https://developer.microsoft.com/nl-nl/windows/downloads/sdk-archive
Copy link
Member

@MarcelRaad MarcelRaad Apr 9, 2018

Choose a reason for hiding this comment

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

en-us would probably be understood by more people than nl-nl ;-)

@kdekker
Copy link
Contributor Author

@kdekker kdekker commented Apr 9, 2018

You are right. Unfortunately, the MS website is doing this 'automagically'.
But how to continue now? As advised, I dropped the original fork and created a new one.

If this change was already committed on the master branch of the cURL repository, I will create a new PR and fix this URL. If this change was not yet committed, I will create a new branch in my own fork, and also create a new PR. If I understood Daniel's comments, this change is as #73070e8 on the master?

But as I made already some other mistakes, I really like to get a confirmation.

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Apr 9, 2018

Ah, you're right, this is already in master. You will get it by doing a git pull --ff-only on your master branch. After that, you can create a new branch from that. Thanks!

kdekker pushed a commit to kdekker/curl that referenced this issue Apr 9, 2018
Follow up on curl#2472.
Now using en-us instead of nl-nl as language code in the URL.
@kdekker
Copy link
Contributor Author

@kdekker kdekker commented Apr 9, 2018

Fixed now by filing a new PR (see above).

MarcelRaad pushed a commit that referenced this issue Apr 9, 2018
Follow up on #2472.
Now using en-us instead of nl-nl as language code in the URL.

Closes #2475
@lock lock bot locked as resolved and limited conversation to collaborators Jul 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants