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: clean target cannot be called #2455

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@kdekker
Contributor

kdekker commented Apr 4, 2018

Due to the check in Makefile.vc:313:

!IF "$(CFGSET)" == "FALSE"
!ERROR please choose a valid mode
!ENDIF

and the condition at line 429:

#######################

Only the clean target can be used if a config was not provided.

!IF "$(CFGSET)" == "FALSE"
clean:
...
!ENDIF

the clean target was unreachable.

No nmake call can be invoked unless a build-type was specified. The clean target only existed when a build type was specified. Now added a clean target unconditional in MakefileBuild.vc + added a clean target in Makefile.vc that propagates to MakefileBuild.vc.

winbuild: clean target cannot be called
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.

@bagder bagder added the build label Apr 4, 2018

@bagder

This comment has been minimized.

Member

bagder commented Apr 5, 2018

(The two red travis builds have been fixed on master in the mean time)

I'd appreciate a comment/review on this take from someone using the winbuild system!

@kdekker

This comment has been minimized.

Contributor

kdekker commented Apr 5, 2018

Thanks Daniel. Can I submit new pull requests for the same file in the meanwhile?

And one thought from my side: if (in the theoretical case) no CFGSET was false then the clean rule is (after my change) the only rule that exists (unconditionally now) in in the MakefileBuild.vc, then if no target was specified upon calling nmake, the clean rule is called. This can be solved too of course. IMO it is quite strange to have targets that only exist under conditions. From my UNIX/Linux experience, this is very uncommon. I would prefer unconditional rules in any case. This is IMO normal practice. And checks should be done to check consistency of settings. Please share your thoughts....

@bagder

This comment has been minimized.

Member

bagder commented Apr 5, 2018

Thanks Daniel. Can I submit new pull requests for the same file in the meanwhile?

Absolutely, and if it depends on this PR, just say so in the message.

it is quite strange to have targets that only exist under conditions

I would agree. But I don't want to step on toes in areas where I personally am not very familiar with (development on Windows) so I don't have any strong opinions here.

setenv command does not exist anymore
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).
@kdekker

This comment has been minimized.

Contributor

kdekker commented Apr 5, 2018

Please forgive my innocence: I continued in my own forked curl repo and made another change (commit + push). If I now create a new pull request, it also shows this pull request, however, this change is not related. How can I isolate a commits in a PR? I only want to create a PR for last commit (now, I also need to make more changes, that are dependent on this PR). Any help is welcome.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Apr 5, 2018

The problem is that you have your changes on the master branch. Just use separate branches for them.

@kdekker

This comment has been minimized.

Contributor

kdekker commented Apr 5, 2018

@MarcelRaad: thanks for reply. I will try to do so (I'm knowing clearcase and its branch concepts, but I'm new to git). But as soon as a previous pull request is merged to the main branch, then I can continue making a new PR based on what then is or will be committed on the master branch?

@rodwiddowson

This comment has been minimized.

Contributor

rodwiddowson commented on 985c140 Apr 5, 2018

The trouble here is that SetEnv as a tool comes and goes with the tide. For instance it is not available to me with a Visual Studio 2017 prompt for instance (as in what you get when I have said

"d:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\vc\Auxiliary\Build\vcvars32.bat"

My suggestion would be to not try to double guess the MS devs. One has to assume that the person reading this understands the tools so why not say

"Establish the VC build environment you need in the canonical[1] way for the tool version you are using. Note that this has changed over time"

[1] http://www.hacker-dictionary.com/terms/canonical : So maybe a bit too nerdy for mainline documentation...

This comment has been minimized.

Contributor

kdekker replied Apr 5, 2018

In Visual Studio you have vcvarsxxx.bat but also VsDevCmd.bat. I did not yet create a pull request, but I made some minor modifications to make BUILD_WINDOWS.txt slightly more up-to-date. See: master...kdekker:winbuild-doc. If you find these changes useful, I will create a PR (assuming you can access the previous URL)

@rodwiddowson

This comment has been minimized.

Contributor

rodwiddowson commented Apr 5, 2018

I'm a lurker, but yea. Both changes (cleaning up the doco and making clean work) look useful. I also know (I think) where you are going with this and I'll be standing by tio test your changes as they come along as patches.

@kdekker

This comment has been minimized.

Contributor

kdekker commented Apr 5, 2018

Thanks Rod: Now created PR: #2459.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Apr 5, 2018

You typically never commit anything to master yourself, see https://github.com/curl/curl/wiki/push-access-guidelines#do-not-work-in-your-local-master-branch. Instead, you pull from the remote master branch to yours to get the latest version and then commit directly to a new branch on top of that (GUIs typically let you do that in one step, I believe on the command line you have to first create the branch from the updated master and then commit.)

@kdekker

This comment has been minimized.

Contributor

kdekker commented Apr 6, 2018

@MarcelRaad: Thanks for providing that information.

My initial landing page about how to contribute with cURL was https://curl.haxx.se/dev/contribute.html. That page describes 3 methods to make a contribution, but preferred the pull-request method. The information for a PR pointed to https://github.com/curl/curl/pulls (the overview of pull requests, where the help behind the 'new pull request button' points to https://help.github.com/articles/about-pull-requests/, a generic information page). After some struggling, I discovered that I had to make a fork and can create a PR from a fork. After some more struggling, I learned to use branches, to isolate changes. The URL mentioned by you provides even better information, but since I'm already busy, I first like to complete my current changes. Anyhow, it would be great if https://curl.haxx.se/dev/contribute.html would point to https://github.com/curl/curl/wiki/push-access-guidelines#do-not-work-in-your-local-master-branch instead of https://github.com/curl/curl/pulls.

@bagder

This comment has been minimized.

Member

bagder commented Apr 6, 2018

Thanks!

@bagder bagder closed this in 8585026 Apr 6, 2018

@bagder bagder referenced this pull request Apr 6, 2018

Closed

Winbuild doc #2459

@lock lock bot locked as resolved and limited conversation to collaborators Jul 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.