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

[hpx] add some features, fix some bugs #25059

Merged
merged 40 commits into from
Sep 19, 2022
Merged

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Jun 3, 2022

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 38bb87c5571555f1a4f64cb4ed9d2be0017f9fc1 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 27a08f2..f2f7962 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2214,7 +2214,7 @@
     },
     "ffmpeg": {
       "baseline": "4.4.1",
-      "port-version": 13
+      "port-version": 14
     },
     "ffnvcodec": {
       "baseline": "11.1.5.0",
diff --git a/versions/f-/ffmpeg.json b/versions/f-/ffmpeg.json
index a78b82a..31615c0 100644
--- a/versions/f-/ffmpeg.json
+++ b/versions/f-/ffmpeg.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "13204e7f992baf351e3512bfb604ee56d5ca1598",
+      "version": "4.4.1",
+      "port-version": 14
+    },
     {
       "git-tree": "ad64f5ffe64b5fcd97e2e6d98273b70d498d6af0",
       "version": "4.4.1",

@LilyWangLL LilyWangLL added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-bug The issue is with a library, which is something the port should already support labels Jun 6, 2022
@LilyWangLL
Copy link
Contributor

Thanks for your PR, please run:

    vcpkg x-add-version ffmpeg

@LilyWangLL LilyWangLL added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Jun 6, 2022
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: checked-in files for ffmpeg have changed but the version was not updated
version: 4.4.1#17
old SHA: b11307cbddac36667d9a47f26679b1851301122b
new SHA: 9b3b9cc4baa69c67dff446ca880aa29e705a5b12
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

versions/f-/ffmpeg.json Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Sep 1, 2022
LilyWangLL
LilyWangLL previously approved these changes Sep 5, 2022
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Sep 5, 2022
)

set(HPX_WITH_MALLOC system)
if(VCPKG_TARGET_IS_LINUX)
# This is done at the request of the hpx maintainers; see
Copy link
Member

Choose a reason for hiding this comment

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

I want to see a demonstration that the problem fixed by using tcmalloc here is not broken by attempting to use jemalloc instead.

Also, this is not a feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/STEllAR-GROUP/hpx/blob/bc49eebc393c95e80192a3c858a342593c2439ae/CMakeLists.txt#L167-L171

https://github.com/STEllAR-GROUP/hpx/blob/bc49eebc393c95e80192a3c858a342593c2439ae/CMakeLists.txt#L191-L197

The default has been changed to tcmalloc on linux and i am just using that.

Also, this is not a feature.

Yeah I know but options RFC is not coming anytime soon.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I know but options RFC is not coming anytime soon.

That doesn't mean that it is OK to use features incorrectly to get such effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't mean that it is OK to use features incorrectly to get such effects.

I don't see a maintainer rule that would disallow the current way the jemalloc feature is implemented.

reason for jemalloc: https://github.com/STEllAR-GROUP/tutorials/tree/master/hlrs2017/session3#dependencies-3

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a maintainer rule that would disallow the current way the jemalloc feature is implemented.

I don't see how 'do not use features to implement alternatives' could be any clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how 'do not use features to implement alternatives' could be any clearer.

This is only meant to apply if alternatives are implemented using different but conflicting features. E.g. if I would have implemented a tbb and a jemalloc feature which would have been mutually exclusive. That would break the additive rule of features.

But however I already removed the feature since i can simply pass the option via VCPKG_CMAKE_CONFIGURE_OPTIONS.

versions/f-/ffmpeg.json Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Sep 8, 2022
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response info:reviewed Pull Request changes follow basic guidelines labels Sep 9, 2022
@dan-shaw dan-shaw merged commit 2b35366 into microsoft:master Sep 19, 2022
@Neumann-A Neumann-A deleted the refine_hpx branch September 19, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants