Skip to content

[scripts-audit] nmake buildsystem#20987

Merged
strega-nil-ms merged 6 commits intomicrosoft:masterfrom
strega-nil:nmake_buildsystem
Nov 5, 2021
Merged

[scripts-audit] nmake buildsystem#20987
strega-nil-ms merged 6 commits intomicrosoft:masterfrom
strega-nil:nmake_buildsystem

Conversation

@strega-nil
Copy link
Copy Markdown
Contributor

These are all simplifications and should not introduce changes in behavior, except for minor or obviously wrong cases (not restoring ENV{CL} after a debug build, for example)

hoping not to make too many changes, just minor simplifications
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 2169ab765b49cfc5cd7eddfc8ff3e579326776f8 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 08c5a89..a77d586 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2338,7 +2338,7 @@
     },
     "gdal": {
       "baseline": "3.3.2",
-      "port-version": 1
+      "port-version": 2
     },
     "gdcm": {
       "baseline": "3.0.7",
diff --git a/versions/g-/gdal.json b/versions/g-/gdal.json
index af74f90..f16bf4c 100644
--- a/versions/g-/gdal.json
+++ b/versions/g-/gdal.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "70e38b6c708d5daa8299f6f1cc88162d5af913f2",
+      "version-semver": "3.3.2",
+      "port-version": 2
+    },
     {
       "git-tree": "6e90412cd51170a5ea63a0067005bb3afc3c6c36",
       "version-semver": "3.3.2",

Copy link
Copy Markdown
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Wouldn't it make sense to also set ENV{LIB}? Finding libs matching release/debug is needed in most ports.

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Oct 26, 2021

FTR I'm about to touch most nmake builds near port gdal, to use pkg-config for dependencies. If you need improvements in argument usage, document what is needed. Or provide a new vcpkg-nmake port to be adopted during port updates.

@strega-nil-ms
Copy link
Copy Markdown
Contributor

@dg0yt it may make sense, but this is not a behavior changing PR in order to make it easier to merge.

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Oct 28, 2021

I keep looking at the source to verify what variables it passes to nmake via command line or environment. Shouldn't this be in the documentation? (How could I know that there is a INSTALLDIR variable?)

@strega-nil-ms
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Copy Markdown
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

nit nit nit nit nit

if(NOT _bn_LOGFILE_ROOT)
set(_bn_LOGFILE_ROOT "build")
if(arg_NO_DEBUG)
message(WARNING "NO_DEBUG argument to ${CMAKE_CURRENT_FUNCTION} is deprecated")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
message(WARNING "NO_DEBUG argument to ${CMAKE_CURRENT_FUNCTION} is deprecated")
message(WARNING "The NO_DEBUG argument to ${CMAKE_CURRENT_FUNCTION} is deprecated")

else()
set(MAKEFILE_NAME ${_bn_PROJECT_NAME})
if(arg_ADD_BIN_TO_PATH)
message(WARNING "ADD_BIN_TO_PATH argument to ${CMAKE_CURRENT_FUNCTION} is deprecated - it never did anything")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
message(WARNING "ADD_BIN_TO_PATH argument to ${CMAKE_CURRENT_FUNCTION} is deprecated - it never did anything")
message(WARNING "ADD_BIN_TO_PATH argument to ${CMAKE_CURRENT_FUNCTION} is deprecated - it had no effect")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No "The" to add here?

endif()

if(NOT VCPKG_HOST_IS_WINDOWS)
message(FATAL_ERROR "${CMAKE_CURRENT_FUNCTION} only support windows.")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
message(FATAL_ERROR "${CMAKE_CURRENT_FUNCTION} only support windows.")
message(FATAL_ERROR "${CMAKE_CURRENT_FUNCTION} only supports windows")

else()
set(MAKEFILE_NAME ${_bn_PROJECT_NAME})
if(arg_ADD_BIN_TO_PATH)
message(WARNING "ADD_BIN_TO_PATH argument to ${CMAKE_CURRENT_FUNCTION} is deprecated - it never did anything")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No "The" to add here?

@strega-nil-ms strega-nil-ms merged commit efdf934 into microsoft:master Nov 5, 2021
@strega-nil strega-nil deleted the nmake_buildsystem branch March 31, 2025 16:13
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.

7 participants