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

[new port] squirrel #18681

Merged
merged 33 commits into from
Nov 5, 2021
Merged

[new port] squirrel #18681

merged 33 commits into from
Nov 5, 2021

Conversation

shybovycha
Copy link
Contributor

@shybovycha shybovycha commented Jun 28, 2021

  • What does your PR fix?

Fixes #14013 by adding squirrel port

  • Which triplets are supported/not supported? Have you updated the CI baseline?

Tested on Windows and OSX Big Sur successfully

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

@ghost
Copy link

ghost commented Jun 28, 2021

CLA assistant check
All CLA requirements met.

@NancyLi1013 NancyLi1013 self-assigned this Jun 28, 2021
@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jun 28, 2021
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/vcpkg.json Outdated Show resolved Hide resolved
ports/squirrel/vcpkg.json Show resolved Hide resolved
Copy link
Contributor Author

@shybovycha shybovycha left a comment

Choose a reason for hiding this comment

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

Added all the requested changes, accordingly to the situation

versions/s-/squirrel.json Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Show resolved Hide resolved
@shybovycha
Copy link
Contributor Author

@NancyLi1013 so what is the process to merge this PR?

@NancyLi1013
Copy link
Contributor

@NancyLi1013 so what is the process to merge this PR?

Since this PR is still in progress to review, it may be take some time. After we reviewed it, our team will merge it.

ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Show resolved Hide resolved
ports/squirrel/portfile.cmake Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

@shybovycha Could you please address the review suggestions?

Copy link

@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 7e5cfccf60b545829e35c0cba5e14049487b997d -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index cc1ba00..42a4aaa 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -6377,7 +6377,7 @@
       "port-version": 1
     },
     "squirrel": {
-      "baseline": "2021-06-29",
+      "baseline": "2021-10-01",
       "port-version": 0
     },
     "sratom": {
diff --git a/versions/s-/squirrel.json b/versions/s-/squirrel.json
index d5b6a22..e417c52 100644
--- a/versions/s-/squirrel.json
+++ b/versions/s-/squirrel.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "0e3ae0f83950ee38f8467d595b5e1dedc8e8ad02",
+      "version-date": "2021-10-01",
+      "port-version": 0
+    },
     {
       "git-tree": "85815f40f1a954639c81da4d2afb64880fd14fc7",
       "version-date": "2021-06-29",

Copy link

@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 7e5cfccf60b545829e35c0cba5e14049487b997d -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index cc1ba00..3158358 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -6377,7 +6377,7 @@
       "port-version": 1
     },
     "squirrel": {
-      "baseline": "2021-06-29",
+      "baseline": "2021-09-17",
       "port-version": 0
     },
     "sratom": {
diff --git a/versions/s-/squirrel.json b/versions/s-/squirrel.json
index d5b6a22..37d6e60 100644
--- a/versions/s-/squirrel.json
+++ b/versions/s-/squirrel.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "bf903aafd156901aef8dfdb92a81b84504940867",
+      "version-date": "2021-09-17",
+      "port-version": 0
+    },
     {
       "git-tree": "85815f40f1a954639c81da4d2afb64880fd14fc7",
       "version-date": "2021-06-29",

Copy link

@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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for squirrel but no changes to version or port version.
-- Version: 2021-09-17
-- Old SHA: bf903aafd156901aef8dfdb92a81b84504940867
-- New SHA: a46e3ef7285f62a731a102e2c0bc274e8d5adb56
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

versions/s-/squirrel.json Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
ports/squirrel/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

Have you tested the feature interpreter? @shybovycha

Copy link

@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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for squirrel but no changes to version or port version.
-- Version: 2021-09-17
-- Old SHA: a46e3ef7285f62a731a102e2c0bc274e8d5adb56
-- New SHA: a1ee51758a6203816497ed3ca01a14291037ac54
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@shybovycha
Copy link
Contributor Author

@NancyLi1013

Have you tested the feature interpreter? @shybovycha

yes, just double-checked with both feature on and off. had to incorporate the logic from vcpkg_copy_tools which cleans up the tools for the case when the feature is disabled (since it will be built anyways by squirrel)

ports/squirrel/portfile.cmake Show resolved Hide resolved
ports/squirrel/portfile.cmake Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

CMake Error at CMakeLists.txt:53 (target_compile_definitions):
  Cannot specify compile definitions for target "sq" which is not built by
  this project.


CMake Error at CMakeLists.txt:53 (target_compile_definitions):
  Cannot specify compile definitions for target "sq_static" which is not
  built by this project.

Currently, since the targets sq and sq_static is only built when BUILD_SQ is on. So the condition is still required in this part.

Add versions

Fix squirrel portfile according to new standards

Add version to squirrel

Remove all files from /bin directory

Update squirrel version

Could not fix the execs in bin/ directory hence simply enforced dir removal

Update versions

Remove obsolete version

Co-authored-by: NancyLi1013 <[email protected]>

Remove trailing comma

Remove bin files only when not using squirrel as library

Do not remove bin dir when on win32 and dynamic linking because of dlls

Fixing the bin directory cleanup for various platforms

Update versions

Smart remove bin directory for static vs dynamic build; add note on the binaries there

Update version

Define 'interpreter' feature which controls building sq and sq_static binaries

Update version and revision of squirrel to the most recent one

Update versions database

Remove bin/ directory for static build

Update versions database

Apply suggestions from code review

Co-authored-by: NancyLi1013 <[email protected]>

Update versions

Fix interpreter tool

Update versions

Add patch to optionally build squirrel interpreter

Update versions

Conditionally build sq and sq_static

Update versions
@shybovycha
Copy link
Contributor Author

@NancyLi1013 just curious: locally vcpkg installs the ports without any issue on two of my machines. i do vcpkg remove squirrel followed by vcpkg install squirrel and optionally specify the [interpreter] suffix to check how the sq tool is built. i even tried --recurse option to force rebuild when installing it. still i have not seen an issue like on CI. i bet the problem is my environment does not do a clean build. do you know how can i enforce a clean build of a tool to reproduce the issue locally?

@shybovycha
Copy link
Contributor Author

@NancyLi1013 could you please review the changes? For some reason GitHub can't show / resolve your request for change, despite the changes are there

@NancyLi1013
Copy link
Contributor

Sorry for the long delay @shybovycha. I must have missed your message before.

Seems you have solved the problems and also tested the feature. Could you please help confirm if the format of EOF for the patch is LF?

Thanks.

@shybovycha
Copy link
Contributor Author

@NancyLi1013 the line endings for the patches are CRLF on my end

@NancyLi1013
Copy link
Contributor

@NancyLi1013 the line endings for the patches are CRLF on my end

Thanks for your conformation. Could you please help update these as LF? Since there might be some problems using CRLF.

@shybovycha
Copy link
Contributor Author

@NancyLi1013 done

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Nov 2, 2021
@dan-shaw dan-shaw merged commit 637c236 into microsoft:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] Squirrel
4 participants