Error message under Angular2: Cannot find module 'engine.io-client'

Do you want to request a feature or report a bug?

What is the current behavior?
ERROR in ./src/app/campaign/campaign.component.ts
Module build failed: Error: /Users/Max/Development/bmt/bmt-ui/angular/src/app/campaign/campaign.component.ts (6,22): Cannot find module 'engine.io-client'.)
    at _checkDiagnostics (/Users/Max/Development/bmt/bmt-ui/angular/node_modules/@ngtools/webpack/src/loader.js:145:15)
    at /Users/Max/Development/bmt/bmt-ui/angular/node_modules/@ngtools/webpack/src/loader.js:173:17
 @ ./src/app/campaign/campaign.module.ts 12:0-57
 @ ./src/app/app.module.ts
 @ ./src/main.ts
 @ multi main

If the current behavior is a bug, please provide the steps to reproduce.

Created an Angular 2 project with angular-cli

angular-cli: 1.0.0-beta.25.5
node: 6.9.1
os: darwin x64
@angular/common: 2.4.4
@angular/compiler: 2.4.4
@angular/core: 2.4.4
@angular/forms: 2.4.4
@angular/http: 2.4.4
@angular/platform-browser: 2.4.4
@angular/platform-browser-dynamic: 2.4.4
@angular/router: 3.4.4
@angular/compiler-cli: 2.4.4

Install engine.io-client module with npm
3.Import engine.io-client in angular component (import * as eio from 'engine.io-client';)
Add engine.io-client.js file to Scripts area in angular-cli.json

"scripts": [
       "../node_modules/engine.io-client/engine.io.js"
     ]

What is the expected behavior?
If this is a feature request, what is motivation or use case for changing the behavior?
Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.

Report this to @ngtools/webpack


Dashboard endpoint for testing prototype

Branch with draft code: https://github.com/gnosis/safe-client-gateway/tree/feature/dashboard_endpoint
Problem statement: https://github.com/gnosis/safe-pm/issues/50

Review document with results https://gnosisinc.slack.com/archives/C01GL8X8L3H/p1647510129876509


join udp multicast group?

When I have the sender send directly to the IP address of the device where UE is running then everything works great. Ideally I'd like send to a multicast address for a bunch of different devices to listen to. I tried <IP_ADDRESS> and the UE receiver gets some of the packets but it looks like there is a lot of packet loss.

Not sure how the underlying udp system handles UDP multicasts, you'd need to dig into e.g. https://docs.unrealengine.com/4.26/en-US/API/Runtime/Sockets/ISocketSubsystem/. Keep in mind UDP is an unreliable stream.


S3 delete file delete/create dir

@mihaimaruseac
This PR adds delete/create dir and delete file for s3.

@shivaylamba Respectfully, I'd ask to refrain from approving PRs when the PR was already approved and the review you submitted has no power to trigger the CI/merge (as you are not an owner of the code / in the organisation). Note the grayed checkmark:

Your review in this case blocked CI and there was a need for another review to unblock. Please don't approve random PRs on GitHub.


Package broken under Python 3.9

The epath sub-package is broken under Python 3.9, apparently due to the recent introduction of syntax only supported in Python 3.10 or later in binary_import.py:
import etils.epath

...
File ~/condahome/miniconda3/envs/py39t/lib/python3.9/site-packages/etils/epy/binary_import.py:55
     49       return True
     50   return False
     53 @contextlib.contextmanager
     54 def binary_adhoc(
---> 55     restrict: None | py_utils.StrOrStrList = None,
     56     verbose: bool = False,
     57     **kwargs: Any,
     58 ) -> Iterator[None]:
     59   yield

TypeError: unsupported operand type(s) for |: 'NoneType' and '_UnionGenericAlias'

Thank you for reporting. Could you provide more context how you encountered this bug ?
The last released version of etils explicitly require Python 3.10 and released on PyPI: https://github.com/google/etils/blob/5e3dbb7977f9d59e8b7bc66a93f20ac38d681ef5/pyproject.toml#L5

PyPI should have raised an error when trying to install the package, so I'm not sure how you were able to install the packaged on 3.9 in the first place.

Closing this. our package has the right metadata. If other packages system install a package not supported by the current python version, this should be reported in conda-forge. There's nothing we can do from the etils side.

FYI this is now effectively meaning I need to bump python to use tensorflow 2.15, as tensorflow-datasets depends on etils. Currently seeing if I can pin etils to an earlier version.

I would suggest you to raise this issue in conda. Our metadata clearly indicates the package is only available for Python 3.10+.
If conda does not respect this and still install new version of etils on an older Python version, it's definitely not our fault and nothing we can do about.

Irrespective of the issues with the conda metadata, I'm just saying that if you strictly need python 3.10 and above, this now means anyone wanting to use tensorflow 2.15 under certain circumstances will also not be able to use python3.9 without manually downgrading etils.


Tensorrt c++ code

I appreciate your exceptional work. May I inquire if there are plans to release a TensorRT C++ version of the code in the future?

Hi, thanks for your interest. Given the fact that I'm currently unfamiliar with TensorRT and occupied with other commitments, that version may not appear in the near future.
But I'm always interested in new technologies and I've heard about TensorRT a lot. So it's very likely that I'll work on it when my schedule clears up a bit. No promises though (as I can't predict the future).


example of using Hnsw in C++

Could you give some examples for using Hnsw in C++, I couldn't find the example in this project.

Hi @Damcy I will add HNSW to this example: https://github.com/nmslib/nmslib/blob/master/sample_standalone_app/sample_standalone_app1.cc
However, you can just substitute, e.g., small_world_rand with hnsw as well as to **modify some query-time and index-time parameters".

@searchivarius Thank you very much! I will try it in these two days.


[CT-3288] [Feature] Remove werkzeug as dependency in 1.4.latest, 1.5.latest

Is this your first time submitting a feature request?

[X] I have read the expectations for open source contributors
[X] I have searched the existing issues, and I could not find an existing issue for this feature
[X] I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature
It appears to me that the werkzeug dependency in unused in the 1.4.latest and 1.5.latest branches. It was removed in 1.6 in 73a0dc6d147923573faaa0e13d51995b2d2caddd
From looking into the repo history, it looks like Werkzeug was added when introducing the RPC server back around version 0.14. My guess is that this was changed along the way and it went unnoticed that werkzeug was no longer used, but I am not certain about that. Perhaps it's been left for some indirect reason that I can't see in just searching this codebase.
I'd like to remove, if possible, due to a dependabot alert received:

Werkzeug DoS: High resource usage when parsing multipart/form-data containing a large part with CR/LF character at the beginning

Describe alternatives you've considered
Werkzeug could also be upgraded, but since it looks like the dependency is unused, removing it would be better.
Who will this benefit?
Anyone receiving this dependabot alert.
Are you interested in contributing this feature?
yes - it's an easy change to make as long as it's acceptable
Anything else?
No response

Thanks for opening this @buck-omni !
If you open PR(s) for this, we can pass this along to our engineering team for final review.

We have the same issue with another Werkzeug dependabot on DBT 1.8.0
it looks like CI failed to run on the PR for some reason, is that blocking this?
https://github.com/dbt-labs/dbt-core/pull/8964


Tabler UI not working?

I have integrated the admin into my project. I was able to create first admin user with /admin/init and see the admin user in the database.
When I go to <IP_ADDRESS>:8000/admin/login I can see a login page. I enter username and password. I get logged in but see empty page with 404 Not Found.
I found another ticket which shows that this project is only for admin back-end API. The front-end needs to be installed separately.
That's OK but in that case README.md is very confusing and it should be changed.

It should remove all screenshots (which suggest that the package comes with full admin UI).
It should clearly say that this project is admin back-end only and has no UI. It should point to the front-end UI repository
It would also be helpful to mention, that GET /init/ page is one time use only - it stops working after creating the first admin user.
Finally, the GET /admin/login page should be removed. It now renders what looks like a regular login page, which makes an impression the project has UI, but logging in leads to 404 and googling leads to a ticket where it says there is no UI and it needs to be installed separately.

Actually, it does provide UI like Django Admin, but you just need to do some steps which I also cannot find in documentation. Refer to https://github.com/fastapi-admin/fastapi-admin/issues/119#issuecomment-1450211260 to follow the steps


Clean-up to match coding standards

/cc @steved @grosser

rename gemfiles using gemfiles/railsX.Y.gemfile like any other repo
kick appraisal for wwtd
remove support for ruby 1.9 and rails 2.3
use common.rb in gemfiles
introduce bump
remove if/else blocks for rails 2
fix test for latest rails 4.2 (return "" instead of " ")
sudo: false

Nice, thanks

Released as 2.0


container widget values don't match docker stats

At least CPU utilization and memory utilization don't match docker stats command.

Deploying PCP 3.10.8 to test.

Cpu Utilization is working now. https://github.com/Netflix/vector/commit/948124b12ba0d51b22f05b7770919317a8eefe3a


⚠ ClamAV has degraded performance

In 8e4fce9, ClamAV (https://spamassassin.apache.org/) experienced degraded performance:

HTTP code: 200
Response time: 73 ms

Resolved: ClamAV performance has improved in fd6aea5 after 539 days, 2 hours, 37 minutes.


Events not fired via websockets when using web3.eth.sendSignedTransaction

Context
Currently I'm using 1.0.0-beta.18
Connected via
new Web3.providers.WebsocketProvider('ws://localhost:8545')
Expected Behavior
When using web3.eth.sendSignedTransaction and using websocket connection events should fire as soon as the transaction is mined.
Current Behavior
When I try to use using web3.eth.sendSignedTransaction, the transaction is mined successfully, but the events emmited by the contract do not get fired but if I refresh my application and reconnect to ganache-cli events are received as expected.
But if I use myContract.methods.myMethod().send everything works fine, events get fired when the block is mined, without reconnecting to the node.
result when using web3.eth.sendSignedTransaction
{
   "hash":"0xa78dfa11c19ba8e40476a16016f2d84894364cb289996c0a76dffbd1a4b70d64",
   "nonce":32,
   "blockHash":"0x677be9f216ee49e1334bb8e738c53ef60044cecbc7a73cb5e8409438e69d695d",
   "blockNumber":906,
   "transactionIndex":0,
   "from":"0x63C728caA8C9bbb1bb08dcbFE0132d6eDf03cbB6",
   "to":"0xc6F361795efF1b64dB552790b633158bc01e8d08",
   "value":"0",
   "gas":2000000,
   "gasPrice":"20000000000",
   "input":"0xb5b4f659000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000087361646661736466000000000000000000000000000000000000000000000000"
}

Steps to Reproduce
Function call
web3.eth.sendSignedTransaction('0x' + signedTransaction, function (err, hash) {
    if (err) {
        return callback(err);
    } else {
        return callback(null, hash);
    }
})

Result
{
   "hash":"0xa78dfa11c19ba8e40476a16016f2d84894364cb289996c0a76dffbd1a4b70d64",
   "nonce":32,
   "blockHash":"0x677be9f216ee49e1334bb8e738c53ef60044cecbc7a73cb5e8409438e69d695d",
   "blockNumber":906,
   "transactionIndex":0,
   "from":"0x63C728caA8C9bbb1bb08dcbFE0132d6eDf03cbB6",
   "to":"0xc6F361795efF1b64dB552790b633158bc01e8d08",
   "value":"0",
   "gas":2000000,
   "gasPrice":"20000000000",
   "input":"0xb5b4f659000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000087361646661736466000000000000000000000000000000000000000000000000"
}

Ganache Logs
ganache         | eth_sendRawTransaction
ganache         | eth_getTransactionReceipt
ganache         | eth_subscribe
ganache         |
ganache         |   Transaction: 0xa78dfa11c19ba8e40476a16016f2d84894364cb289996c0a76dffbd1a4b70d64
ganache         |   Gas usage: 1226406
ganache         |   Block Number: 906
ganache         |   Block Time: Thu Feb 01 2018 02:01:51 GMT+0000 (UTC)
ganache         |
ganache         | eth_getTransactionReceipt
ganache         | eth_unsubscribe

Event listening
contract.events[eventName]({
    fromBlock: 0
}).on('data', function (event) {
    <--NOT CALLED-->
}).on('changed', function (event) {
    <--NOT CALLED-->
}).on('error', function (err) {
    console.log(err);
});

I'm also experiencing this issue, though I can't see any reference to sendSignedTransaction; I'm finding this with sendRawTransaction, which is invoked with the library I'm using (https://github.com/ethers-io/ethers.js/).
I have been able to get this to work with an older version of Ganache (version 1.0.1 of the UI, not sure which version of ganache-core it's using), so I might have to do a bisect to find out the cause.

Closing due to age/inactivity.
Please open a new issue if you're still having problems. Thanks!


EngineTest.ScenarioStreamRedirect fails with a LookupException

It looks like this is due to the the standard library not being available and so none of the codecs are available via _codecs.lookup. The failure occurs even though the particular encoding causing the failure (cp437) is available to use via bytes.decode/str.encode. This is because io.TextIOWrapper does not make use of the StringOps.TryGetEncoding mechanism but instead relies on PythonOps.LookupEncoding.
Related to https://github.com/IronLanguages/ironpython2/issues/659 (probably the same issue)
@BCSharp not sure if this something you encountered before?

Yes, I was aware of that. This is one of the main reasons why several tests at the top of test_codecs_stdlib are not passing yet. I am not quite sure what is the best course of action though. Here are some of my musings on this topic.

Python documentation of codecs reads:

While the builtin open() and the associated io module are the recommended approach for working with encoded text files, this module provides additional utility functions and classes that allow the use of a wider range of codecs when working with binary files.

This tells me that there is not much need to use text encodings in codecs directly, as long as open() and io are optimized to use .NET encodings, which should be the case anyway for performance reasons. codecs seems to me a legacy from Python 2.

But from the compatibility point of view, the codecs module should be available and complete. The quickest way to ensure this is to provide a proxy class to any .NET Encoding that can impersonate a Python codec by providing a codecs-like interface (codecs.CodecInfo, codecs.IncrementalEncoder, codecs.IncrementalDecoder, codecs.StreamReader, codecs.StreamWriter). And if not "the quickest" then at least the most comprehensive. There are some challenges with this approach (e.g. preserving the   state of incremental encoding/decoding is done differently in .NET than Python), but once done, all .NET codecs become available through codecs. Remaining codecs written in pure Python top it up and there is only a small number of codecs left out that would have to be manually addressed.

However, a few of .NET codecs behave slightly differently than the corresponding Python codecs, which makes me hesitant to fully embrace the above approach. So far I have noticed the discrepancies in UTF-7 and a few charmap codecs (I am ignoring discrepancies due to bugs in .NET for now). The UTF-7 case is probably acceptable, for two reasons: (1) both implementations conform to the standard and are correct, meaning that output produced by the .NET encoder can be decoded by CPython and vice versa; (2) CPython's UTF-7 codecs is written in C, with utf_7.py providing just a Python proxy, so unless IronPython reimplements the complete codec in C#, the .NET behaviour is as good as it gets.
The charmap encodings story is a little bit different. In come codepages, some byte values do not have defined mapping to characters. Encountering such bytes in the input, the CPython codec will raise a decoding error. The .NET encoding will (usually? always?) map it successfully to a character using Latin-1. This also works during encoding. Charmap encodings (except charmap itself) are pure Python so using codecs guarantees the exact CPython behaviour, in case somebody needs it. On the other hand, it could be surprising to have a different behaviour between, say:
b'\x81'.decode('windows-1252')

and
codecs.decode(b'\x81', 'windows-1252')

as it is now. Also, charmap encodings from codecs are significantly slower than their .NET counterparts (except for Latin-1, of course).

An alternative approach would be to fully implement codecs the Python way and redirect str.encode and bytes.decode to use them to avoid discrepancies. This would require some effort as many non-trivial codecs depend on C/C# modules for the actual workload. The effort in this direction seems to have started in the past (there is a skeleton _codecs_cn.cs and _multibytecodecs.cs, for example). Besides the effort, the drawback is a performance hit.

But unless Python StdLib gets patched, this may not be enough: many codec proxies written in Python assume that Python strings are in UTF-32. Since in IronPython strings are UTF-16, incremental encoding will stay broken across a wide range of codecs (as it is now). Ironically, some of the remaining functionality of codecs that is not obsolete in Python 3 is support of incremental encoding/decoding.

"unless Python StdLib gets patched" => or unless #252 gets implemented.

Currently I am leaning towards the approach described in point 2 (with all its caveats), but since there are still other loose ends in the encoding support, I pursue them first and maybe more clarity will emerge later. A discussion could help too.


Detect filename is not compliant with PascalCase convention

Hi,
I found that FilenameRule does not check PascalCase convention mentioned in https://kotlinlang.org/docs/coding-conventions.html#source-file-names
What do you think?
Thx
Ivos

hi @bedla thanks for the PR. Reading the Kotlin docs, my take is that filenames should always conform to Pascal case, regardless of the contents. Is that not also your understanding?

Yep, from my understanding PascalCase is only viable name of the file.
I have rebased to current master.

I have update PR to current master

The code seems unnecessary complex to me. Basically, I would expect it do the following:

Determine the number of top level declarations
Determine the name of the first class
If one top level declaration was found and the first class name is not null than the name of file should be identical to the name of the class followed by ".kt". Otherwise check if the filename adheres to the the PascalCase convention.

@bedla Do you plan to follow up on my remarks on this PR?

Yes, I will. Sorry for being late, I had some rush in work.

Just pushed requested changes. I have rewritten original logic to make it more explicit from my point of view. Pls, take a look. Thx

Hmm, interesting "edge cases".

CommandLineExt & ByteArrayExt - actually I like more this convention with DescriptiveName+Ext.kt ... could we extends original Kotlin's convention with exception to Ext suffix in this case? It seems reasonable to me.
ToolchainForTests - I agree with rule that filename should be Project.kt, but on the other hand there should be possible to add exceptions from the rule.

What do you thing?

CommandLineExt & ByteArrayExt - actually I like more this convention with DescriptiveName+Ext.kt ... could we extends original Kotlin's convention with exception to Ext suffix in this case? It seems reasonable to me.

ToolchainForTests - I agree with rule that filename should be Project.kt, but on the other hand there should be possible to add exceptions from the rule.

I think we should handle both cases similar. If a file does not contain any class and has filename ending with "Ext" or "Extension" and is following the PascalCase convention then there should be no violation about the file name. Would that be feasible?

Ok, I will take a look at it. I have also created https://youtrack.jetbrains.com/issue/KT-51919 to know Kotlin creator's opinion on this edge case (I do not know if they will join discussion, but why not try :) ).

Mr. Elizarov responded in Youtrack issue. I think that he is right.
But it means that should remove checks around allOfSameReceiver. And also his response invalidate what was mentioned in original code https://github.com/pinterest/ktlint/blob/0.45.2/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/FilenameRule.kt#L67 ... If I get it correctly, it is because of situation when we want to name file with same name like Receiver name, when we have only extensions in the file. But this one is not explicitly noted in lang-docs.
What do you think?
Do you know why exactly is that note at line 67?

It looks like that https://github.com/pinterest/ktlint/blob/0.45.2/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/FilenameRule.kt#L67 is just the example of a file in which extension functions have the same receiver. There can be a good reason to group such functions together in one file, but I agree that we should not force that the file is named after the receiver. It would be enough to enforce the upper PascalCase convention here.
When applying above, a project can still use a filename like SomeReceiverExt.kt as it adheres to the upper PascalCase convention. But the other way around, a file which only contain extension functions on the Foo receiver does not necessarily to be named as Foo.kt but could be named as RedundantBar.kt.

@bedla Do you have some time to finish up the issue? I expect a new release to be build in a couple of weeks.

ahh, sorry I missed your previous comment.

I have removed that all-top-level receivers exception, but it still fails on:
C:\Users\ivo.smid\IdeaProjects\ktlint\ktlint\src\main\kotlin\com\pinterest\ktlint\internal\ByteArrayExt.kt:1:1: Extension property hex should be declared in a file named hex.kt (cannot be auto-corrected) (filename)
C:\Users\ivo.smid\IdeaProjects\ktlint\ktlint\src\main\kotlin\com\pinterest\ktlint\internal\CommandLineExt.kt:1:1: Extension function printHelpOrVersionUsage should be declared in a file named printHelpOrVersionUsage.kt (cannot be auto-corrected) (filename)

So if I get it correctly, I should remove also this sub-rule for Extension functions & Extension property? What do you think?

Is it desired that with this change, the following file:
package com.vanniktech.feature.boardmoney.data

import app.cash.sqldelight.adapter.primitive.IntColumnAdapter
import app.cash.sqldelight.db.SqlDriver
import com.vanniktech.boardmoney.QueryWrapper

internal fun createQueryWrapper(driver: SqlDriver) = QueryWrapper(
  driver = driver,
  boardMoneyPlayerAdapter = BoardMoneyPlayer.Adapter(
    colorAdapter = IntColumnAdapter,
  ),
)

gets linted with the following:

/Users/niklas/dev/GitHub/vanniktech/feature-board-money/src/commonMain/kotlin/com/vanniktech/feature/boardmoney/data/Db.kt:1:1: File 'Db.kt' contains a single top level declaration and should be named 'CreateQueryWrapper.kt' (cannot be auto-corrected) (filename)

That does not seem right to me.

If a Kotlin file contains a single class or interface (potentially with related top-level declarations), its name should be the same as the name of the class, with the .kt extension appended. It applies to all types of classes and interfaces. If a file contains multiple classes, or only top-level declarations, choose a name describing what the file contains, and name the file accordingly. Use upper camel case with an uppercase first letter (also known as Pascal case), for example, ProcessDeclarations.kt.

I'd agree for a Class / Interface, but not for a given function (as in my case).


V1.4.2

Fix saved state

Run a test on circle ci

Use the library to upload to bintray

Bug fix #34

I found this issue.
I'll merge.


:bug: Fix: Bug with state persistence not working in forked mode

Description
Concise description of proposed changes
Testing
Explain the quality checks that have been done on the code changes
Additional Information

[ ] I read the contributing docs (if this is your first contribution)

Your ENS/address:

[!WARNING]
This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

#1056  👈
#1055
#1054
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @roninjin10 and the rest of your teammates on  Graphite


RFC: Initial support of idmapped mount points

Hi,this is this is initial support of idmapped mount points in containerd. The original PR was published by @mauriciovasquezbernal here #4734.Updates:Original PR was rebased on top of latest containerd.Idmapped mount support was reimplemented using in Go.Small changes in error handling.Added some comments.Updated container start time performance measurement results (numbers are in seconds).

Image
ID mapped mount
WithRemappedSnapshot

BusyBox
00.135
04.964

Ubuntu
00.171
15.713

Fedora
00.143
38.799

In current patch set the only idmapped mount points functionality is implemented. I think that user namespace support for containerd should be integrated separately, please share your opinion, maybe I am wrong here. I don't know if anybody is working on it, if no I would like to work on user namespaces support also.Dear reviewers, could you please check this PR.Thank you in advance!cc @alban @rata @mauriciovasquezbernal @AkihiroSuda

Cant wait :)

Previous PR's: #5075 #4734 #5888

/retest

@kzys Thanks for comment! Fixed

Looks like CGroup V2 tests fail is unrelated to this patches, but will recheck.

Thanks Mauricio, rebased, comments were fixed.

@artqzn asked me to comment here. I'm currently working on support for overlayfs but that will still take a little while. The first step is to extend the idmapping infrastructure to handle filesystems mounted inside a userns. I have sent patches for this which I aim to get into 5.16. Thanks!

From the Kubernetes POV (I'm working on a KEP with @giuseppe for userns support un k8s) what will be great if:

The chown needed for the container base image when running with userns can be avoided (we will need that the fs in the host having the rootfs has id mapped mounts support and that it is allowed to use an id mapped mount as lowerdir in a overlayfs, which is not true currently IIUC).
We will need to add support in containerd for the metacopy overlayfs parameter, to have an acceptable storage and performance overhead, until id mapped mounts can be used for this (the metacopy makes sense anyways, as not all users will have new enough kernels and supported fs). This is unrelated to this PR, of course, just mentioning it for completion.
Have support to use id mapped mounts for volumes. But this is not important in the short term (we will do other things first) and will probably need changes in the OCI runtime-spec and runc (see https://github.com/opencontainers/runtime-spec/pull/1090 for an open discussion on the runtime-spec part).

@artqzn just curious, what do you mean with:

In current patch set the only idmapped mount points functionality is implemented. I think that user namespace support for containerd should be integrated separately, please share your opinion, maybe I am wrong here. I don't know if anybody is working on it, if no I would like to work on user namespaces support also.

Will this PR not avoid the chown needed at the beginning if user namespaces is used?

From the Kubernetes POV (I'm working on a KEP with @giuseppe for userns support un k8s) what will be great if:

The chown needed for the container base image when running with userns can be avoided (we will need that the fs in the host having the rootfs has id mapped mounts support and that it is allowed to use an id mapped mount as lowerdir in a overlayfs, which is not true currently IIUC).

Yes, currently it is impossible to mount overlayfs on top of idmapped lowerdir. There are a patch from @brauner in Linux kernel that disables this.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v5.15.5&id=029a52ada6a774cac457f3be34b79cfc85563ced

We will need to add support in containerd for the metacopy overlayfs parameter, to have an acceptable storage and performance overhead, until id mapped mounts can be used for this (the metacopy makes sense anyways, as not all users will have new enough kernels and supported fs). This is unrelated to this PR, of course, just mentioning it for completion.

Yes, not related but definitely would be good improvement. I can try to make a POC for this and share results.

Have support to use id mapped mounts for volumes. But this is not important in the short term (we will do other things first) and will probably need changes in the OCI runtime-spec and runc (see Support mount_setattr(2) attributes (mostly for recursively-readonly mounts) opencontainers/runtime-spec#1090 for an open discussion on the runtime-spec part).

@artqzn just curious, what do you mean with:

In current patch set the only idmapped mount points functionality is implemented. I think that user namespace support for containerd should be integrated separately, please share your opinion, maybe I am wrong here. I don't know if anybody is working on it, if no I would like to work on user namespaces support also.

Will this PR not avoid the chown needed at the beginning if user namespaces is used?

Current patch add support of idmapped mounts only for overlayfs snapshotter and ctr tool can be used to create containers with idmapped mounts.
But this PR doesn't include CRI part that should be updated also. In PR  #4734 there are experimental support for user namespace in containerd with three options POD, CLUSTER and NODE. These patches:

https://github.com/containerd/containerd/pull/4734/commits/977049e3b642be502b494c41e30a2efcd4a1da97
https://github.com/containerd/containerd/pull/4734/commits/bbc5afde4d8c6fd91ac0a6367a2ba370bd74cb49

But looks like that time there was no agreement regarding interface that CRI should support.
So, this PR implements overlayfs snapshotter support for idmapper mounts, but CRI should be also updated to enable idmapped mounts and user namespace for containers.

Yes, currently it is impossible to mount overlayfs on top of idmapped lowerdir. There are a patch from @brauner in Linux kernel that disables this.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v5.15.5&id=029a52ada6a774cac457f3be34b79cfc85563ced

Oh, thanks for the link!

We will need to add support in containerd for the metacopy overlayfs parameter, to have an acceptable storage and performance overhead, until id mapped mounts can be used for this (the metacopy makes sense anyways, as not all users will have new enough kernels and supported fs). This is unrelated to this PR, of course, just mentioning it for completion.

Yes, not related but definitely would be good improvement. I can try to make a POC for this and share results.

Oh, if you want to help with this, it will be great! Please cc me on the PR and feel free to reach out to me on slack or anything if you want :)

Will this PR not avoid the chown needed at the beginning if user namespaces is used?

Current patch add support of idmapped mounts only for overlayfs snapshotter and ctr tool can be used to create containers with idmapped mounts. But this PR doesn't include CRI part that should be updated also.

Ohhh, I see what you meant now. Yes, it is perfectly fine to not include the CRI part. For the 1. item I mentioned, CRI should not be involved, for the second either, and only for the third (and we need to see how the runtime-spec will look like, etc. first). So, we really can't do any CRI changes regarding id mapped mounts yet. So, this is great as it is :)

In PR #4734 there are experimental support for user namespace in containerd with three options POD, CLUSTER and NODE. These patches:

Yes, I worked with Mauricio on the k8s part of that. But the new revisited KEP we are working on will not include the "modes" at all, it won't be needed to send any of that to the container runtime (and most things will be automatic and simplified). So, no need to worry about those things for now :)

So, this PR implements overlayfs snapshotter support for idmapper mounts, but CRI should be also updated to enable idmapped mounts and user namespace for containers.

As I said, this is perfect and we can't do much more than that yet :)

Thank you!

Thank you! :)

Oh, if you want to help with this, it will be great! Please cc me on the PR and feel free to reach out to me on slack or anything if you want :)

Ok, thanks, I've created an issue for this and will implement PoC.
https://github.com/containerd/containerd/issues/6310
Sure, will contact you, just to maybe clarify if more things are necessary from the idmapped mounts support in containerd for Kubernetes.

Yes, I worked with Mauricio on the k8s part of that. But the new revisited KEP we are working on will not include the "modes" at all, it won't be needed to send any of that to the container runtime (and most things will be automatic and simplified). So, no need to worry about those things for now :)

Yes, I've read it, thanks, this is great!

Can we have integration tests?

Can we have integration tests?

Sure, will check how to implement them due to I am new in containerd, thanks for pointing this!
The only one thing here. The integration tests will fail due to there is no support of idmapped mounts in Liinux kernel yet. As Christian told it is expected in 5.17.
But I can implement integration tests and run them on my PC and add to the pathset.

Rebased, many thanks to @AlexeyPerevalov for integration test implementation and suggestions!

Can we have integration tests?

Sure, will check how to implement them due to I am new in containerd, thanks for pointing this!
The only one thing here. The integration tests will fail due to there is no support of idmapped mounts in Liinux kernel yet. As

For overlayfs there isn't. But for ext4, xfs, btrfs, and fat.

Christian told it is expected in 5.17. But I can implement integration tests and run them on my PC and add to the pathset.

The vfs bits have landed for v5.17, i.e. the generic changes needed to start porting overlayfs. I'm currently rebasing and starting the overlayfs port.

Can we have integration tests?

Sure, will check how to implement them due to I am new in containerd, thanks for pointing this!
The only one thing here. The integration tests will fail due to there is no support of idmapped mounts in Liinux kernel yet. As

For overlayfs there isn't. But for ext4, xfs, btrfs, and fat.

Christian told it is expected in 5.17. But I can implement integration tests and run them on my PC and add to the pathset.

The vfs bits have landed for v5.17, i.e. the generic changes needed to start porting overlayfs. I'm currently rebasing and starting the overlayfs port.

@brauner thank you for clarifications, but I've much more question. Your patchset contains selftest, it uses open_tree -> mount_setattr -> move_mount approch as well as github.com/brauner/mount-idmapped. And this approach works well on latest kernels with bind mount on ext4 fs. But I didn't see any samples with mount  syscall-> mount_setattr.
This PR uses mount -> mount_setattr and for overlayfs on kernel built with patchset<EMAIL_ADDRESS>- it works. But I didn't manage to make mount -> mount_setattr work on ext4 (bind mount) on kernel v5.13 and on your initial patchset (I know mount_setattr syscall has been changed). Do you have a samples for mount -> mount_setattr case?

Can we have integration tests?

Sure, will check how to implement them due to I am new in containerd, thanks for pointing this!
The only one thing here. The integration tests will fail due to there is no support of idmapped mounts in Liinux kernel yet. As

For overlayfs there isn't. But for ext4, xfs, btrfs, and fat.

Christian told it is expected in 5.17. But I can implement integration tests and run them on my PC and add to the pathset.

The vfs bits have landed for v5.17, i.e. the generic changes needed to start porting overlayfs. I'm currently rebasing and starting the overlayfs port.

@brauner thank you for clarifications, but I've much more question. Your patchset contains selftest, it uses open_tree -> mount_setattr -> move_mount approch as well as github.com/brauner/mount-idmapped. And this approach works well on latest kernels with bind mount on ext4 fs. But I didn't see any samples with mount syscall-> mount_setattr. This PR uses mount -> mount_setattr and for overlayfs on kernel built with patchset<EMAIL_ADDRESS>- it works. But I didn't manage to make mount -> mount_setattr work on ext4 (bind mount) on kernel v5.13 and on your initial patchset (I know mount_setattr syscall has been changed). Do you have a samples for mount -> mount_setattr case?

There's some code here:
https://github.com/brauner/mount-idmapped
and there's a full-blown generic vfs testsuite here:
https://github.com/kdave/xfstests/blob/master/src/idmapped-mounts/idmapped-mounts.c
then there's also the examples from my manpage:
https://man7.org/linux/man-pages/man2/mount_setattr.2.html

Can we have integration tests?

Sure, will check how to implement them due to I am new in containerd, thanks for pointing this!
The only one thing here. The integration tests will fail due to there is no support of idmapped mounts in Liinux kernel yet. As

For overlayfs there isn't. But for ext4, xfs, btrfs, and fat.

Christian told it is expected in 5.17. But I can implement integration tests and run them on my PC and add to the pathset.

The vfs bits have landed for v5.17, i.e. the generic changes needed to start porting overlayfs. I'm currently rebasing and starting the overlayfs port.

@brauner thank you for clarifications, but I've much more question. Your patchset contains selftest, it uses open_tree -> mount_setattr -> move_mount approch as well as github.com/brauner/mount-idmapped. And this approach works well on latest kernels with bind mount on ext4 fs. But I didn't see any samples with mount syscall-> mount_setattr. This PR uses mount -> mount_setattr and for overlayfs on kernel built with patchset<EMAIL_ADDRESS>- it works. But I didn't manage to make mount -> mount_setattr work on ext4 (bind mount) on kernel v5.13 and on your initial patchset (I know mount_setattr syscall has been changed). Do you have a samples for mount -> mount_setattr case?

There's some code here:
https://github.com/brauner/mount-idmapped
and there's a full-blown generic vfs testsuite here:
https://github.com/kdave/xfstests/blob/master/src/idmapped-mounts/idmapped-mounts.c
then there's also the examples from my manpage:
https://man7.org/linux/man-pages/man2/mount_setattr.2.html
I found that current upstream kernel
https://github.com/torvalds/linux/blob/e6251ab4551f51fa4cee03523e08051898c3ce82/fs/namespace.c#L3964
checks whether target mount point was published or not, and in case when it was pulished sys_mount_setattr returns EINVAL, so since mount syscall publishes mount point, I guess, now mount_setattr works only with open_tree and move_mount.
There wasn't such check in<EMAIL_ADDRESS>

Hi, there is several a questions regarding this patchset to all of you guys and regarding kernel functionality to Christian Brauner (@brauner).
Currently the idmapped mounts are applied to already published mount point. So, overlayfs snapshotter creates overlayfs mount point and containerd applies GID and UID shifts on top of it using mount_setattr() system call.
But according to @brauner example here https://github.com/brauner/mount-idmapped, the idmapped mounts should be applied to "anonymous"/"detached" mount. I the example the open_tree() system call is used to get "detached" mount file descriptor.
The question is next:
Looks like that it should be done in containerd in the same way, but it is impossible to use open tree gracefully for overlayfs, due to we no need to change the mount point location. I've checked and the approach with fsopen(), fsconfig(), fsmount() and move_mount() should work. In this approach we create detached overlayfs mount by using fsopen() and fsconfig(). Then we can apply GID/UID shift by using mount_setattr() and "publish" overlayfs mount with already applied idmapping.
int main(void) {
        int target_fd;
        int userns_fd;
        int overlay_fd;
        int mfd;
        int ret;
        struct mnt_attr attr = {
                .attr_set = MOUNT_ATTR_IDMAP,
                .attr_clr = 0,
                .propagation = 0,
                .userns = 0,
        };

        overlay_fd = sys_fsopen("overlay", FSOPEN_CLOEXEC);
        if (overlay_fd < 0) {
                printf("Failed to open overlay filesystem:errno=%d\n", errno);
                return overlay_fd;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "source", "overlay", 0);
        if (ret < 0) {
                printf("Failed to set source path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "lowerdir", "/test/lowerdir", 0);
        if (ret < 0) {
                printf("Failed to set lowerdir path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "upperdir", "/test/upperdir", 0);
        if (ret < 0) {
                printf("Failed to set upperdir path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "workdir", "/test/workdir", 0);
        if (ret < 0) {
                printf("Failed to set workdir path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
        if (ret < 0) {
                printf("Failed to create overlays config:errno=%d\n", errno);
                return ret;
        }

        mfd = sys_fsmount(overlay_fd, FSMOUNT_CLOEXEC, 0);
        if (mfd < 0) {
                printf("Failed to create overlayfs mount fd:errno=%d\n", errno);
                return mfd;
        }

        attr.userns = open("/proc/"pid_in_user_ns"/ns/user", O_RDONLY | O_CLOEXEC | O_NOCTTY);
        if (attr.userns < 0) {
                printf("Failed to create user namespace:errno=%d\n", errno);
                return attr.userns;
        }

        ret = sys_mount_setattr(mfd, "", AT_EMPTY_PATH | AT_RECURSIVE, &attr, sizeof(attr));
        if (ret < 0) {
                printf("Failed to change mount attributes:errno=%d\n", errno);
                return ret;
        }

        ret = sys_move_mount(mfd, "", -EBADF, "/test/merged", MOVE_MOUNT_F_EMPTY_PATH);
        if (ret < 0) {
                printf("Failed to publish mount point:errno=%d\n", errno);
                return ret;
        }

        return 0;
}

This approach seems better than current one. Unfortunately we still need proxy binary to create id mappings.
I think it is better to reimplement this patches using "detached" mount approach. Also, I am not sure if currently implemented approach will be supported by overlayfs, I mean that looks like that mount_setattr() will work only with "detached" mounts but here I am not sure. Maybe @brauner can answer this question.
Thank you in advance!

Hi, there is a question regarding this patchset to all of you guys and regarding kernel functionality to Christian Brauner (@brauner).
Currently the idmapped mounts are applied to already published mount point. So, overlayfs snapshotter creates overlayfs mount point and containerd applies GID and UID shifts on top of it using mount_setattr() system call. But according to @brauner example here https://github.com/brauner/mount-idmapped, the idmapped mounts should be applied to "anonymous"/"detached" mount. I the example the open_tree() system call is used to get "detached" mount file descriptor.
The question is next: Looks like that it should be done in containerd in the same way, but it is impossible to use open tree gracefully for overlayfs, due to we no need to change the mount point location. I've checked and the approach with fsopen(), fsconfig(), fsmount() and move_mount() should work. In this approach we create detached overlayfs mount by using fsopen() and fsconfig(). Then we can apply GID/UID shift by using mount_setattr() and "publish" overlayfs mount with already applied idmapping.
int main(void) {
        int target_fd;
        int userns_fd;
        int overlay_fd;
        int mfd;
        int ret;
        struct mnt_attr attr = {
                .attr_set = MOUNT_ATTR_IDMAP,
                .attr_clr = 0,
                .propagation = 0,
                .userns = 0,
        };

        overlay_fd = sys_fsopen("overlay", FSOPEN_CLOEXEC);
        if (overlay_fd < 0) {
                printf("Failed to open overlay filesystem:errno=%d\n", errno);
                return overlay_fd;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "source", "overlay", 0);
        if (ret < 0) {
                printf("Failed to set source path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "lowerdir", "/test/lowerdir", 0);
        if (ret < 0) {
                printf("Failed to set lowerdir path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "upperdir", "/test/upperdir", 0);
        if (ret < 0) {
                printf("Failed to set upperdir path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_SET_STRING, "workdir", "/test/workdir", 0);
        if (ret < 0) {
                printf("Failed to set workdir path:errno=%d\n", errno);
                return ret;
        }

        ret = sys_fsconfig(overlay_fd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
        if (ret < 0) {
                printf("Failed to create overlays config:errno=%d\n", errno);
                return ret;
        }

        mfd = sys_fsmount(overlay_fd, FSMOUNT_CLOEXEC, 0);
        if (mfd < 0) {
                printf("Failed to create overlayfs mount fd:errno=%d\n", errno);
                return mfd;
        }

        attr.userns = open("/proc/"pid_in_user_ns"/ns/user", O_RDONLY | O_CLOEXEC | O_NOCTTY);
        if (attr.userns < 0) {
                printf("Failed to create user namespace:errno=%d\n", errno);
                return attr.userns;
        }

        ret = sys_mount_setattr(mfd, "", AT_EMPTY_PATH | AT_RECURSIVE, &attr, sizeof(attr));
        if (ret < 0) {
                printf("Failed to change mount attributes:errno=%d\n", errno);
                return ret;
        }

        ret = sys_move_mount(mfd, "", -EBADF, "/test/merged", MOVE_MOUNT_F_EMPTY_PATH);
        if (ret < 0) {
                printf("Failed to publish mount point:errno=%d\n", errno);
                return ret;
        }

        return 0;
}

This approach seems better than current one. Unfortunately we still need proxy binary to create id mappings. I think it is better to reimplement this patches using "detached" mount approach. Also, I am not sure if currently implemented approach will be supported by overlayfs, I mean that looks like that mount_setattr() will work only with "detached" mounts but here I am not

Once overlayfs has gained support for idmapped mounts - this is still in the works - the vfs api should be useable in the same way as with the other fses that already support it.

@artqzn overlayfs support for idmap mount has landed in 5.19 now. Do you want to resurrect this PR now?

@artqzn overlayfs support for idmap mount has landed in 5.19 now. Do you want to resurrect this PR now?

Yes, will do, maybe it will take some time, due to I think that it is necessary to move it on top of open_tree() and move_mount(), or fs_mount(). Need to revisit the implementation.

Is this what will enable Docker / docker compose to better support the feature out of the box? I think Podman has implemented some sort of support for it in their volume mount options already.
Are there blockers presently, or just a lack of time / review?

Should we now rely on OCI runtime for this?

https://github.com/opencontainers/runtime-spec/pull/1143
https://github.com/opencontainers/runc/pull/3717

@AkihiroSuda yes, exactly, we need containerd to use idmap mounts for the rootfs

@artqzn do you plan to work on this in the short term? It will be great to merge this

@rata, I'm going to answer instead, we talked with @artqzn and came to a conclusion that this support of ID mapped mounts should not use mount fd API by itself only, I mean containerd should support new mount API first, then we'll be able to properly implement support for ID mapped mounts using fsopen/fsmount/fsconfig/etc. So, this PR should be merged as it works well for time being.
I'm going to create a new issue for this, also I would be glad to discuss it in slack with community if they're opened for this feature :)

So, @IlyaHanov do you plan to rebase this on latest main to fix the conficlts and push for this to be merged? I can try to help if you don't have the time

Sure, hope I'll push changes next week

So, @IlyaHanov do you plan to rebase this on latest main to fix the conficlts and push for this to be merged? I can try to help if you don't have the time

Rebased

@IlyaHanov I think the PR can just use openTree() to create a detached mount, instead of os.Open(). This function is present in the unix package.

Fixed
PTAL, @rata, @cpuguy83, @kzys

@IlyaHanov This commit added a capability we could use to remap ids (i.e. use idmap mounts): 31a6449
Can you use that whenever the fs support it?

I'll take a look at this in 1 week. I'm sorry, but I can't properly answer now, need time to check

@rata @IlyaHanov I did a poc https://github.com/fuweid/go-mountidmapped/blob/692c64ef1200715be86ef4c148eebddbc7570b67/pkg/sys/userns.go#L34
Please take a look. Hack go-runtime as what we did before in https://github.com/containerd/containerd/pull/2502

@rata @IlyaHanov I did a poc https://github.com/fuweid/go-mountidmapped/blob/692c64ef1200715be86ef4c148eebddbc7570b67/pkg/sys/userns.go#L34
Please take a look. Hack go-runtime as what we did before in https://github.com/containerd/containerd/pull/2502

@fuweid, thank you for this, looks interesting. I'll give a try, actually I'm interesting in benchmarks in containerd's context, once I prepare measurements I'll post them, going to play with your solution, thanks!

@IlyaHanov This commit added a capability we could use to remap ids (i.e. use idmap mounts): https://github.com/containerd/containerd/commit/31a6449734f167b465a000e87e6cad2dd2a0abc5
Can you use that whenever the fs support it?

@rata, thanks for waiting, sure, snapshotter should report this capability whenever the underlying fs supports id-mapped mounts, I'll prepare changes using this capability.
Also I have a few questions related to that commit, just a few clarifications about expected workflow/implementation:

If snapshotter doesn't support id-mapped mounts -- we'll prepare/chown/commit snapshot and use that snapshot as a parent for new one. Snapshotter acts as usual -- nothing changed.
if snapshotter does support id-mapped mounts -- it should prepare internally another id-mapped snapshot and report its mounts? I'm asking this, since in resolveSnapshotOptions raw parent key is returned, and then prepare is called, am I right?

I can try to help if you don't have the time

May I ask you few questions about containerd historical/user namespace use-cases and may be possible implementation directly via email or anything else you'd prefer to use?

actually I'm interesting in benchmarks in containerd's context,

Hi @IlyaHanov I didn't follow it. Did you mean the idmapped performance on mounting? or fork-exec process?
If I understand correctly, this patch doesn't work if the image has more than 2 layers. The test case is using busybox image which has only one layer. The overlayfs snapshot returns bind mount option instead of overlay. Check the following code.
https://github.com/containerd/containerd/blob/5885db62c856e703f11f2195c54efbec5d00a788/snapshots/overlay/overlay.go#L437-L448
And the overlayfs doesn't support FS_ALLOW_IDMAP so we can't mount_setattr on overlayfs mountpoint directly. We need to mount_setattr lowerdir one by one before overlayfs mount. Fortunately, the tmp mount point can be unmounted after overlayfs mounted.

Hi @IlyaHanov I didn't follow it. Did you >mean the idmapped performance on >mounting? or fork-exec process?

I meant fork-exec process, containerd is much larger binary than /bin/true, so I'm interested in memory consumption numbers.
Anyway, forking /bin/true should be avoided, at least, due to possible race condition (there may already be no child process).

If I understand correctly, this patch doesn't work

You understand correctly, this patch doesn't work at all. Originally, I didn't have much time, but now I see, it won't work as it is for time being, not only because it tries to id-map merged directory.
That's why I asked two those questions -- I'm going to re-imolement this and It'll be great for me to know expected workflow, because I don't know the whole story, I mean how it worked before.
I'll do my best to prepare fresh workable version in next two weeks.

I meant fork-exec process, containerd is much larger binary than /bin/true, so I'm interested in memory consumption numbers.

Did you mean re-exec? If so, it is not a good way in containerd. We don't want re-exec in the containerd process.

Anyway, forking /bin/true should be avoided, at least, due to possible race condition (there may already be no child process).

Just wonder the solution about no child process! I'm trying to make it forkable with golink which used by exec package. It works right now. If you need any help or question about this, please ping me in slack. Thanks!

I thought you wanted to use fork-exec approach to avoid forking /bin/true, may be I missed something?

Sorry I make unclear comment. I did fork but not exec. Just fork a process for waiting parent writes the idmap and exit.
We don't want to introduce dependency of extra binary during running. IMO, it will introduce cost of maintaining binary.
I know it is pain to fork in go-runtime. But we need try to keep it in one process.

Take a look at https://github.com/cpuguy83/gonso/blob/2e4618d6a2831021f74df74e3a322f4c1d700353/set.go#L288-L306
re: forking semantics for userns.

@IlyaHanov @cpuguy83 @fuweid I had a look on what we can do, to avoid the helper that paths seems the way to go. We can even use or copy this, that is already doing that correctly (setting pdeathsignal, etc.): https://github.com/containers/storage/blob/6e387ad2a232df73d0a1ec2eae3b91cf525f786d/pkg/idmap/idmapped_utils.go#L80-L115
@IlyaHanov regarding your questions, it really depends on how we define it. I think I'd look at the code and see what is reasonable to do or not. But knowing if the code to achieve it would be reasonable, I'd say:

Probably yes. Maybe warning that this has a huge storage and container startup latency overhead.
I'm honestly not familiar with that to answer. I guess maintainers are best to answer here (or just do what you think seems best).

@IlyaHanov @cpuguy83 @fuweid I had a look on what we can do, to avoid the helper that paths seems the way to go. We can even use or copy this, that is already doing that correctly (setting pdeathsignal, etc.): https://github.com/containers/storage/blob/6e387ad2a232df73d0a1ec2eae3b91cf525f786d/pkg/idmap/idmapped_utils.go#L80-L115

Great, thank you for this clarification, it looks much better than helper-binary, certainly, will give it a try!

@IlyaHanov regarding your questions, it really depends on how we define it. I think I'd look at the code and see what is reasonable to do or not. But without knowing if the code to achieve it would be reasonable or not, I'd say:

I have an idea how to implement this, just need to take a closer look at few use cases and what containerd already does in case of chown. Anyway, thank you, once I finish implementation I'll tag you, hope will do this in 2 weeks :)

Guys, I finished re-implementation of idmapped mounts and pushed it to my own repository due to now we have a few issues with Artem's repository. I and @artqzn are both trying to solve them, but if you're interested in to take a look at the implementation, please, visit https://github.com/IlyaHanov/containerd/commits/idmapped_mounts. Once those issues are solved I'll push to this branch.
Cc @rata @fuweid @cpuguy83

@fuweid , thanks for the review!

For the fork-process solution, I think we should follow the go-runtime pattern, which we use it in several releases. The pattern is to link the syscall beforeFork/AfterFork/AfterForkInChild, which introduced in https://github.com/containerd/containerd/pull/2502. And the gvisor is using it as well link. BeforeFork is used to block signals during a fork and handle goroutine-stack things.

I'll modify it this way, thanks. Could you, please, point me out whether containerd has these routines (beforeFork, afterFork, etc.) or not, looks like changes in that PR have been (re)moved?

I think the get_userns_fd is really good design. We don't need to fork child-process for each idmapped-mount. We just need to hold the /proc/xx/ns/user FD before the child-process exits. The FD can be reused during one overlayfs mount, even if we can use it as cache in the future. That is why I make a poc for this and it actually works.

True, I didn't catch it, thanks, will fix!

And I'm not fan to kill the child process without pidfd because of pid recycle. Since it is short-live process, I'd like to let the child process exit by itself. The gonso project goes this way as well.

I see your point, but I think it hardly be a real case, but maybe it's better safe than sorry, so I guess we can use CLONE_PIDFD + pidfd_send_signal here. I'm not sure that synchronization on pipes much better.

Hi @IlyaHanov

Could you, please, point me out whether containerd has these routines (beforeFork, afterFork, etc.) or not, looks like changes in that PR have been (re)moved?

Yes. It has been removed by https://github.com/containerd/containerd/pull/7513 because unshare(CLONE_FS) can work with thread-level. In this case, we don't need to fork-process for the chdir. But for NS_USER, we need fork-process :(.

I see your point, but I think it hardly be a real case, but maybe it's better safe than sorry, so I guess we can use CLONE_PIDFD + pidfd_send_signal here. I'm not sure that synchronization on pipes much better.

For initial support, I think we can handle it next patch. I raise this issue because we did meet pid recycle in production env :).

@IlyaHanov had a quick look at the patch, the SupportsIDMappedMounts() func testing for kernel version is probably not a good idea. Some distros will backport this (I know we want to backport this in my company), usually the way to see if something is supported is just doing the syscall with some example params. If you get EINVAL or things like that, it is not supported, otherwise it is.
You only need to do the open_tree() + mount_setattr() to test if the fs supports idmap (no movemount to really mount it). Although given that this is overlayfs (I haven't tried that), maybe you need to move_mount and create a tmp overlayfs just to see if it supports idmapped layers? That seems kind of painful to test, though, and a risk to leak these dummy mounts :-/
So, in the end, not sure what we want. Definitely testing a kernel version is not ideal, but the alternatives are kind of painful too. Maybe if we fork, create a mount namespace, test there and then just exit? That way we won't leak mounts at least.
@fuweid what do you think?

Hi @IlyaHanov I'm AFK this week. I will take a look on next Monday. Thanks!

Checked that code:

I think that we should not check kernel version for the feature. Some distributions might backport the feature into old stable kernel.
In https://github.com/IlyaHanov/containerd/commit/dcaccee748d34672f2795db64e384caafd935a31, the commit changes the snapshot dir uid/gid. I'm not sure why you try to change this part. The image can be used by root.
I think we should use the fd instead of pid during mounting. I don't want to run into any issue related to pid cycle. So I suggest to open fd for /proc/xxx/ns/user and kill the process.

Overall, I think the code can be RFC state. Thanks.

@fuweid , thanks for the review!

I think that we should not check kernel version for the feature. Some distributions might backport the feature into old stable kernel

Yes, I'm aware of this issue, so I implemented slow path check for idmapped mounts (this commit https://github.com/IlyaHanov/containerd/commit/de00fb5db1c16631241016182dd21d23a8e314a9) -- if the underlying kernel version is greater or equal to 5.19 (that's where idmapped mounts for overlay has gained support)  then we say we have idmapped mounts support for overlayfs, otherwise it goes slow path -- I create directories, mapping, etc. and try to create idmapped mount for overlayfs and then check whether uid/gid changed, if not -- overlayfs has no idmapped mounts support (check this). I assume that kernels that newer or equal to 5.19 has support for idmapped mounts.

In https://github.com/IlyaHanov/containerd/commit/dcaccee748d34672f2795db64e384caafd935a31, the commit changes the snapshot dir uid/gid. I'm not sure why you try to change this part. The image can be used by root.

This is because "upperdir" needs to be remapped too, otherwise rootfs directory will still be owned by initial user (mapping will not be applied, so usually it's root), so if we try to create/remove file/directory from rootfs -- we'll  likely have "Permission Denied". If snapshot is going to be used by root -- nothing changed it will be used by root, since no uidmap/gidmap labels specified.

I think we should use the fd instead of pid during mounting. I don't want to run into any issue related to pid recycle. So I suggest to open fd for /proc/xxx/ns/user and kill the process before mount

IIUC,  you mean to just apply mapping and before we invoke mount operation we first kill the child process, right?

Yes. After apply mapping to child process, we can open the file of child process /proc/xxx/ns/user, which increases the refCount of user namespace in kernel. The kernel will hold the user namespace until we close the fd, even if the child process has already exited before closing fd.

@fuweid, IIUC you want to make a child process to live as short as possible, so I think this solution should work, I can give it a try.
Note that in our case I don't see pid recycling issue (I mean the situation when the pid we have belongs to another process i.e. has been re-assigned), because we control the child's lifetime, the only related issue we can run into is "running out of pids", I mean the system won't be able to create a process due to lack of pids, but this situation may happen only during forking the process, now containerd will just error out if any error during the fork happened, but it's configurable.
Maybe I missed something, so could you, please, clarify, what exactly we want to achieve here?

Hi @IlyaHanov

IIUC you want to make a child process to live as short as possible, so I think this solution should work, I can give it a try.

Yes. This solution is working and the function API will be simple to use, because the caller doesn't need to know the child process.

Note that in our case I don't see pid recycling issue (I mean the situation when the pid we have belongs to another process i.e. has been re-assigned), because we control the child's lifetime, the only related issue we can run into is "running out of pids", I mean the system won't be able to create a process due to lack of pids,

Pid recycling isn't about the lack of pids. And in containerd shim, we have child-process reaper.https://github.com/containerd/containerd/blob/8c21759d7c00acdd81b54ba1007be8022467897b/sys/reaper/reaper_unix.go#L63
The case is that the child-process is killed after writing mapping data. And the shim reaper will receive the SIG_CHILD signal and cleanup the child-process. So, the system can reuse pid. We don't know how long the mount will take. In the high-load system, it's easy to hit the pid-recycling issue. We hit it in 2020 https://github.com/containerd/containerd/pull/4204.
So, I suggest to use short process as possible as we can.

@fuweid , I implemented the approach you have suggested regarding fd on /proc/[pid]/ns/user. I have also added unit tests for idmapped for overlayfs, please, check them.
Regarding CI, as far as I can see, it (rockylinux) failed due to this test:
    default: === RUN   TestPodUserNS
    default: === RUN   TestPodUserNS/userns_uid_mapping
    default:     pod_userns_linux_test.go:110: Create a sandbox with userns
    default: E0522 08:42:06.899674   65704 remote_runtime.go:135] RunPodSandbox from runtime service failed: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount rootfs component: failed to create idmapped mount: Unable to shift GID/UID for /run/user/1000/ovl-idmapped2387320805/0: function not implemented: unknown
    default:     pod_userns_linux_test.go:115: Unexpected RunPodSandbox error: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount rootfs component: failed to create idmapped mount: Unable to shift GID/UID for /run/user/1000/ovl-idmapped2387320805/0: function not implemented: unknown
    default: === RUN   TestPodUserNS/userns_gid_mapping
    default:     pod_userns_linux_test.go:110: Create a sandbox with userns
    default:     pod_userns_linux_test.go:115: Unexpected RunPodSandbox error: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount rootfs component: failed to create idmapped mount: Unable to shift GID/UID for /run/user/1000/ovl-idmapped866984759/0: function not implemented: unknown
    default: E0522 08:42:06.962223   65704 remote_runtime.go:135] RunPodSandbox from runtime service failed: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount rootfs component: failed to create idmapped mount: Unable to shift GID/UID for /run/user/1000/ovl-idmapped866984759/0: function not implemented: unknown
    default: === RUN   TestPodUserNS/rootfs_permissions
    default:     pod_userns_linux_test.go:110: Create a sandbox with userns
    default: E0522 08:42:07.023266   65704 remote_runtime.go:135] RunPodSandbox from runtime service failed: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount rootfs component: failed to create idmapped mount: Unable to shift GID/UID for /run/user/1000/ovl-idmapped3310181018/0: function not implemented: unknown
    default:     pod_userns_linux_test.go:115: Unexpected RunPodSandbox error: rpc error: code = Unknown desc = failed to create containerd task: failed to create shim task: failed to mount rootfs component: failed to create idmapped mount: Unable to shift GID/UID for /run/user/1000/ovl-idmapped3310181018/0: function not implemented: unknown
    default: === RUN   TestPodUserNS/fails_with_several_mappings
    default:     pod_userns_linux_test.go:110: Create a sandbox with userns
    default: E0522 08:42:07.027214   65704 remote_runtime.go:135] RunPodSandbox from runtime service failed: rpc error: code = Unknown desc = failed to generate sandbox container spec: user namespace configuration: UID mapping: only one mapping line supported, got 2 mapping lines
    default: --- FAIL: TestPodUserNS (0.21s)
    default:     --- FAIL: TestPodUserNS/userns_uid_mapping (0.09s)
    default:     --- FAIL: TestPodUserNS/userns_gid_mapping (0.06s)
    default:     --- FAIL: TestPodUserNS/rootfs_permissions (0.06s)
    default:     --- PASS: TestPodUserNS/fails_with_several_mappings (0.00s)

Looks like we got ENOSYS during idmapped mount, which means the kernel these tests are running on has no support for it. Now I'm not sure why this happened, my first thoughts are that something wrong with SupportsIDMappedMounts function, need to check. Also, I'll fix linters on the next iteration. Thanks!

Hi, guys, looks like CI failed due to "blockfile" and "devmapper", not sure in a reason, but may this be related to container-runtimes themselves? I'm asking this because the same tests have been passed with another container runtime. Also, as I can see "vagrant/fedora" failed for the same reason as main branch, so I assume it's not related to this PR.
Cc @fuweid @cpuguy83 @kzys @rata

Should we now rely on OCI runtime for this?~ (We still need containerd-side logic for rootfs?)

https://github.com/opencontainers/runtime-spec/pull/1143
https://github.com/opencontainers/runc/pull/3717

Originally posted by @AkihiroSuda in https://github.com/containerd/containerd/issues/5890#issuecomment-1424578871

@artqzn @IlyaHanov @rata @fuweid WDYT about this? ↑
The current PR seems to affect non-rootfs mounts too.

/ok-to-test

cc @IlyaHanov ^

Hey, @rata, as I mentioned above, I see the only one case when the implementation can affect something else, but rootfs (as far as I remember the implementation of snapshotter/container opts):

@AkihiroSuda , current behavior is identical to WithRemappedSnapshot. The difference is that the only snapshotter knows what to specify for mount options, but it acts the same way. The only way we can "affect" something else -- to pass {uid,gid}map= directly to mount options... but I doubt very strongly that this might be the case. So, I would say that it can only affect the - snapshot's rootfs.

P.S. I'm sorry, but I'm afk now. Anyway, if you have questions -- post them, I'll do my best to answer.

An interesting fact, on my environment that is being run 6.3.0 kernel on, blockfile unit tests keeps failing with the following:
--- FAIL: TestBlockfile (0.06s)
    blockfile_test.go:31: failed to get snapshotter options: failed to mount device /tmp/TestBlockfile4170233415/001/scratch: failed to setup loop with direct: invalid argument
FAIL
FAIL	github.com/containerd/containerd/snapshots/blockfile	0.085s

And dmesg says:
Jul 12 17:34:19 <...>: operation not supported error, dev loop6, sector 204672 op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
Jul 12 17:34:19 <...>: operation not supported error, dev loop6, sector 570 op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
Jul 12 17:34:19 <...>: operation not supported error, dev loop6, sector 13388 op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
Jul 12 17:34:19 <...>: operation not supported error, dev loop6, sector 98306 op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2

At the first glance looks like it is due to tmpfs doesn't support FALLOC_FL_ZERO_RANGE op.
P.S. I tried the same using the latest main branch and caught this too.
Cc @fuweid

@fuweid , you're right, setting TMPDIR to a custom location helped, thanks!

@fuweid , PTAL, CI failures looks to be unrelated

CI failures looks to be unrelated

For the kubernetes one, please rebase. Thanks

/retest

ping @AkihiroSuda @dmcgowan @rata @kzys @cpuguy83 @mikebrow for review

I'll try it out and take a look hopefully tomorrow, the other containerd PRs I was working on took more time than I expected. Sorry for the delay :-D

Looks good to me. But you should address @fuweid's comments.

Could you, please, point out which comments you're talking about, I was sure I addressed all of them

NOTE: I would like to wait for more reviewers' LGTM on this before merging.

The ptrace there makes sure the command is halted just before exec (see https://github.com/golang/go/issues/21428 and here). It halts the program to wait for the tracer to attach, so the program itself is never executed and just halted there, which can resolve most of the pid reuse issues you mention: the process won't finish :).

At the first glance, it looks nice and does all we do. @fuweid @AkihiroSuda WDYT?

Furthermore, we can use exec.CommandContext() that will cancel the command whenever the context is cancelled (so we can create a ctx like ctx, cancel := context.WithCancel(context.Background()) and use that as cleanup func).

Not sure about this part, we kill child process once we have userns fd, so I don't think we need any deferred activity. Also, I'm not sure about "customizing" it, especially it seems there's no way to use pidfd with this (at the first glance, need to check that closer) and https://github.com/containerd/containerd/pull/5890#discussion_r1274418061.
Overall, I think this should be a follow-up PR, since it needs testing, this one is "Initial" part and looks to be stable now.

the enhancement should be handled in follow-up. Yes, the ptrace looks good but we can consider that we should not use re-exec in here. If re-exec was accepted at the beginning, it won't be so hard in here.

the enhancement should be handled in follow-up. Yes, the ptrace looks good but we can consider that we should not use re-exec in here. If re-exec was accepted at the beginning, it won't be so hard in here.

Cool, let's keep it like this, then! :)

@rata I will say "Done" on comments I already addressed, since I can't mark them as "resolved" due to lack of permissions

@IlyaHanov the CI failure is not related, right?

CI failure is not related, right?

Yep, there was some network issue.

Can you re-push so the CI starts again?

Done, bet 10$ it'll fail for unrelated reason :D

cc @metux

@fuweid is there anything that stops this PR from merging or we're waiting for something else (maybe I missed something)?

ping @containerd/committers @containerd/reviewers to review this change. Thanks

CI is unable to run the test.
I also tried to do this manually and noticed its falling back to chown.
I see in docs this is only supported for overlay as of 5.19 (I'm on Jammy @ 5.15).
Getting EINVAL here: https://github.com/containerd/containerd/pull/5890/files#diff-0d4696b29eacc6f2ed899b12982ab6fa275670ee474536e067f8778732211e56R275

failed to mount idmapped overlay to /tmp/ovl-idmapped-check4197491038/merged: invalid argument

Can we use godoc.org/github.com/cpuguy83/gonso instead?

I can give it a try, but I have doubts whether it is safe to always clone with CLONE_FILES. Also, it depends on you (maintainers) to decide which way is better for you (@rata suggested another one which does almost the same work as gonso and this PR). Anyway, it looks like a follow-up, so I think it's better to open a separate PR for it, see https://github.com/containerd/containerd/pull/5890#issuecomment-1663109342.

I also tried to do this manually and noticed its falling back to chown.
I see in docs this is only supported for overlay as of 5.19 (I'm on Jammy @ 5.15).
Getting EINVAL here: https://github.com/containerd/containerd/pull/5890/files#diff-0d4696b29eacc6f2ed899b12982ab6fa275670ee474536e067f8778732211e56R275

Could, you, please, share what you did and what tests you're talking about? I wounder how were you able to get this error, I never print it, just check, it may be logged in future somewhere else.

Could, you, please, share what you did and what tests you're talking about? I wounder how were you able to get this error, I never print it, just check, it may be logged in future somewhere else.

First I just ran a container and found it was chowning.
So I made a CLI binary that runs the overlayutils.SupportsOverlay function.

So I made a CLI binary that runs the overlayutils.SupportsOverlay function.

IIUC, you did something like:
if _, err := overlayutils.SupportsIDMappedMounts(); err != nil {
        // handle error
}

If so, note that error in this case is just an informative thing:
// This function returns error whether the kernel supports idmapped mounts
// for overlayfs or not, i.e. if e.g. -ENOSYS may be returned as well as -EPERM.
// So, caller should check for (true, err == nil), otherwise treat it as there's
// no support from the kernel side.

Ah, of course. Here's the real error.

overlayfs: unrecognized mount option "ro" or missing value

overlayfs: unrecognized mount option "ro" or missing value

Ouch, that's interesting, I need to check whether this behavior is normal on older kernels, because I don't have this error on 5.15.84 kernel or it is just ubuntu's "feature"
Could you, please, try the same with this change:
diff --git a/snapshots/overlay/overlayutils/check.go b/snapshots/overlay/overlayutils/check.go
index c81050e49..04b396f11 100644
--- a/snapshots/overlay/overlayutils/check.go
+++ b/snapshots/overlay/overlayutils/check.go
@@ -270,7 +270,7 @@ func SupportsIDMappedMounts() (bool, error) {
 		}
 	}()

-	opts := fmt.Sprintf("index=off,lowerdir=%s,upperdir=%s,workdir=%s,ro", lowerDir, filepath.Join(td, "upper"), filepath.Join(td, "work"))
+	opts := fmt.Sprintf("index=off,lowerdir=%s,upperdir=%s,workdir=%s", lowerDir, filepath.Join(td, "upper"), filepath.Join(td, "work"))
 	if err = unix.Mount("", filepath.Join(td, "merged"), "overlay", 0, opts); err != nil {
 		return false, fmt.Errorf("failed to mount idmapped overlay to %s: %w", filepath.Join(td, "merged"), err)
 	}

cc @fuweid

👍 That's a better error

idmapped layers are currently not supported

@cpuguy83 , I guess this change should solve your problem:
diff --git a/snapshots/overlay/overlayutils/check.go b/snapshots/overlay/overlayutils/check.go
index c81050e49..bc31233f3 100644
--- a/snapshots/overlay/overlayutils/check.go
+++ b/snapshots/overlay/overlayutils/check.go
@@ -270,8 +270,8 @@ func SupportsIDMappedMounts() (bool, error) {
 		}
 	}()

-	opts := fmt.Sprintf("index=off,lowerdir=%s,upperdir=%s,workdir=%s,ro", lowerDir, filepath.Join(td, "upper"), filepath.Join(td, "work"))
-	if err = unix.Mount("", filepath.Join(td, "merged"), "overlay", 0, opts); err != nil {
+	opts := fmt.Sprintf("index=off,lowerdir=%s,upperdir=%s,workdir=%s", lowerDir, filepath.Join(td, "upper"), filepath.Join(td, "work"))
+	if err = unix.Mount("", filepath.Join(td, "merged"), "overlay", uintptr(unix.MS_RDONLY), opts); err != nil {
 		return false, fmt.Errorf("failed to mount idmapped overlay to %s: %w", filepath.Join(td, "merged"), err)
 	}
 	defer func() {

@cpuguy83 , issue that you pointed out is fixed, I accidentally passed "ro" as mount data to overlayfs, while it should be a mount flag. Thanks for noticing this!

@IlyaHanov Would you please to rebase it? Thanks
ping @AkihiroSuda @dmcgowan if it's acceptable, I would like to merge and keep move it forward.

/retest

@IlyaHanov Would you please to rebase it? Thanks

@fuweid , I rebased it to the latest main. CI failures look to be unrelated

/retest


fix: fix gcloud SDK detection on Windows

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

[x] Make sure to open an issue as a bug/issue before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
[x] Ensure the tests and linter pass
[x] Code coverage does not decrease (if any source code was changed)
[ ] Appropriate docs were updated (if necessary)

Fixes #383

@googlebot I signed it!

Cut a local build and tested with my project, it seems to work as intended.

Thanks for the patch!

@chingor13 thanks for merging.  This is my first time contributing to this project.  What is the release schedule?  Or when can I expect this fix to be released?


ci: add registry-url and provenance to npm publish

Description of changes:
The first attempt at running the action failed, this update applies some of the config from here: https://docs.npmjs.com/generating-provenance-statements#example-github-actions-workflow
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 12.29%. Comparing base (12359b3) to head (530cc24).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #290   +/-   ##
=======================================
  Coverage   12.29%   12.29%
=======================================
  Files        2246     2246
  Lines      191038   191038
  Branches   174951   174951
=======================================
+ Hits        23479    23480    +1
+ Misses     166392   166390    -2
- Partials     1167     1168    +1

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


cartopy cannot find geos library

Issue description
Following this: http://nixos.org/nixpkgs/manual/#handling-dependencies, propagatedBuildInputs should link in run time dependencies.
Steps to reproduce
default.nix:
with import <nixpkgs> {};

(let cartopy = pkgs.pythonPackages.buildPythonPackage rec{
  name = "cartopy-${version}";
  version = "";

  src = pkgs.fetchurl{
  url = "mirror://pypi/c/cartopy/cartopy-0.11.0.tar.gz";
    sha256 = "85eb55d6ce16be5f7e745fcc2870b3fa7450db3a77899daade8b18711532be45";
  };
  doCheck = false;

  propagatedBuildInputs =  [ gcc pkgs.geos proj pythonPackages.numpy  pythonPackages.cffi pythonPackages.cython];
};
in python.buildEnv.override {
  extraLibs = [
    cartopy
    pkgs.geos
    proj
];
  ignoreCollisions = true;
}).env

Running it:
$ nix-shell
[nix-shell:~/]$ python
Python 2.7.13 (default, Dec 17 2016, 20:05:07)
[GCC 5.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import cartopy
Failed `CDLL(libgeos_c.so.1)`
>>>
>>>
[nix-shell:~/]$ LD_LIBRARY_PATH=/nix/store/6ln1621k5rmnvc92dmcyf18614py37va-geos-3.6.1/lib/ python
Python 2.7.13 (default, Dec 17 2016, 20:05:07)
[GCC 5.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import cartopy
>>>

Technical details
[nix-shell:~/]$ nixos-version
17.03.git.ac3cae0 (Gorilla)

[nix-shell:~/]$ nix-env --version
nix-env (Nix) 1.11.8

[nix-shell:~/]$ nix-instantiate --eval '<nixpkgs>' -A lib.nixpkgsVersion
"17.03.git.ac3cae0631"

Related? https://github.com/NixOS/nixpkgs/issues/7307
I think ctypes is trying to load the library at runtime, so somehow I need to set LD_LIBRARY_PATH in my default.nix, preferably without the hash in the /nix/store/..............geos/lib/.
Any ideas?
thanks!

With Python on Nix you cannot rely on ctypes and have to patch code to use an absolute path to a library.
Therefore, in this case, you will have to patch the cartopy code to use an absolute path to refer to libgeos_c.so.1.

The issue here seems to be a bit different since ctypes or cffi doesn't seem to be used. The only references in cartopy to geos I can find are in _trace.h and trace.pyx. If Cython is used then cartopy should be able to find the library during runtime.
I took your Nix expression and built cartopy but got
$ python
Python 2.7.13 (default, Dec 17 2016, 20:05:07)
[GCC 5.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import cartopy
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/nix/store/h48zwwlpy3jf6idsql6svx5xzr32ayi7-python-2.7.13-env/lib/python2.7/site-packages/cartopy/__init__.py", line 23, in <module>
    import shapely.speedups
ImportError: No module named shapely.speedups

The NixOS version you mention is not a revision that is available via https://github.com/NixOS/nixpkgs/. What other changes did you make?

https://github.com/husnoo/nixpkgs/commit/ac3cae063135b1010d118b8d1dd643b422e64465
I only removed the system images from android, as not everyone has a 1 TB hdd.
I did have a few more libraries installed, I'll check later.
so I need to patch cartopy at  build time? it's that in the let.. clause above?

Full default.nix
with import <nixpkgs> {};

(let cartopy = pkgs.pythonPackages.buildPythonPackage rec{
  name = "cartopy-${version}";
  version = "";

  src = pkgs.fetchurl{
  url = "mirror://pypi/c/cartopy/cartopy-0.11.0.tar.gz";
    sha256 = "85eb55d6ce16be5f7e745fcc2870b3fa7450db3a77899daade8b18711532be45";
  };
  doCheck = false;

  propagatedBuildInputs =  [ gcc pkgs.geos proj pythonPackages.numpy  pythonPackages.cffi pythonPackages.cython];
};
in python.buildEnv.override {
  extraLibs = [
    cartopy
    pkgs.geos
    proj
    pkgs.pythonPackages.pygments
    pkgs.pythonPackages.pyaml
    pkgs.pythonPackages.markdown
    pkgs.pythonPackages.numpy
    pkgs.pythonPackages.msgpack
    pkgs.pythonPackages.scipy
    pkgs.pythonPackages.tables
    pkgs.pythonPackages.sqlalchemy
    pkgs.pythonPackages.scikitlearn
    pkgs.pythonPackages.sympy
    pkgs.pythonPackages.pyqt4
    pkgs.pythonPackages.shapely
    pkgs.pythonPackages.pyshp
    pkgs.pythonPackages.tensorflowWithoutCuda
    pkgs.pythonPackages.gnureadline
    pkgs.pythonPackages.h5py
    pkgs.pythonPackages.pandas
    pkgs.pythonPackages.dask
    pkgs.pythonPackages.scrapy
    pkgs.pythonPackages.matplotlib
    pkgs.pythonPackages.statsmodels
    pkgs.pythonPackages.seaborn
    pkgs.pythonPackages.bokeh
    pkgs.pythonPackages.requests2
    pkgs.pythonPackages.jupyter
    pkgs.pythonPackages.ipython
    pkgs.pythonPackages.cffi
    pkgs.pythonPackages.imageio
    pkgs.theano
];
  ignoreCollisions = true;
}).env

I think this issue was closed by accident - but I don't think I can re-open it. Should I submit a new one? thanks!

I attempted to supply the -I and -L paths at build time, but it didn't help.
  setupPyBuildFlags = ["-I${geos}/include" "-L${geos}/lib"];

FRidh, what's the recommended way to patch the path to geos?
thanks!

No, it wasn't closed by accident. You describe an issue with a package that is not part of Nixpkgs tree. Furthermore, I cannot reproduce your issue. I tried again, with nixos-17.03 @ 69d9061908162bd973fcf34d0fc6dc9d9f8cf9ed and again I could not reproduce your issue.
I took the trace and made some fixes, but now another module is needed but missing (shapefile). I'm closing this again until you provide a minimal working example.

Hi, sorry I misunderstood your earlier request. Having deleted most of the libraries above, and then tried it, I had the same problem you mention with shapely, shapefile and six. this example is the smallest I can make it and reproduce the missing geos business:
with import <nixpkgs> {};

(let cartopy = pkgs.pythonPackages.buildPythonPackage rec{
  name = "cartopy-${version}";
  version = "";

  src = pkgs.fetchurl{
  url = "mirror://pypi/c/cartopy/cartopy-0.11.0.tar.gz";
    sha256 = "85eb55d6ce16be5f7e745fcc2870b3fa7450db3a77899daade8b18711532be45";
  };
  doCheck = false;
  setupPyBuildFlags = ["-I${geos}/include" "-L${geos}/lib"];
  propagatedBuildInputs =  [ gcc pkgs.geos proj pythonPackages.numpy  pythonPackages.cffi pythonPackages.cython pythonPackages.shapely pythonPackages.six pythonPackages.pyshp];
};

in python.buildEnv.override {
  extraLibs = [
    cartopy
];
  ignoreCollisions = true;
}).env

[nix-shell:~/nixshellish]$ python
Python 2.7.13 (default, Dec 17 2016, 20:05:07)
[GCC 5.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import cartopy
Failed `CDLL(libgeos_c.so.1)`
>>>

No such problem if I set the LD_LIBRARY_PATH:
[nix-shell:~/nixshellish]$ LD_LIBRARY_PATH=/nix/store/6ln1621k5rmnvc92dmcyf18614py37va-geos-3.6.1/lib/ python
Python 2.7.13 (default, Dec 17 2016, 20:05:07)
[GCC 5.4.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import cartopy
>>>

is this enough information to reopen please?

THanks

Now I can reproduce your issue, thank you.

I think the issue is with Shapely, I seem to be getting errors there with e.g. from shapely.geometry import Point.

Yep, shapely needs a bit more patching. Because it currently has no maintainers I think its up to you to fix it :)

Good catch! Apparently it's already being handled:
[nix-shell:~/data/analysis]$ find /nix/store/ | grep shapely | grep ".py$" | xargs grep libgeos_c.so
/nix/store/644s1aciw6c5mnqsy4l8ls5sca79iin5-python2.7-Shapely-1.5.15/lib/python2.7/site-packages/shapely/geos.py:    _lgeos = load_dll('geos_c', fallbacks=['/nix/store/6ln1621k5rmnvc92dmcyf18614py37va-geos-3.6.1/lib/libgeos_c.so'])
/nix/store/644s1aciw6c5mnqsy4l8ls5sca79iin5-python2.7-Shapely-1.5.15/lib/python2.7/site-packages/shapely/geos.py:        _lgeos = load_dll('geos_c', fallbacks=['/nix/store/6ln1621k5rmnvc92dmcyf18614py37va-geos-3.6.1/lib/libgeos_c.so'])
/nix/store/644s1aciw6c5mnqsy4l8ls5sca79iin5-python2.7-Shapely-1.5.15/lib/python2.7/site-packages/shapely/geos.py:    _lgeos = load_dll('geos_c', fallbacks=['/nix/store/6ln1621k5rmnvc92dmcyf18614py37va-geos-3.6.1/lib/libgeos_c.so'])
/nix/store/644s1aciw6c5mnqsy4l8ls5sca79iin5-python2.7-Shapely-1.5.15/lib/python2.7/site-packages/shapely/geos.py:    _lgeos = load_dll('geos_c', fallbacks=['/nix/store/6ln1621k5rmnvc92dmcyf18614py37va-geos-3.6.1/lib/libgeos_c.so'])
/nix/store/644s1aciw6c5mnqsy4l8ls5sca79iin5-python2.7-Shapely-1.5.15/lib/python2.7/site-packages/shapely/_buildcfg.py:                         fallbacks=['libgeos_c.so.1', 'libgeos_c.so'])
/nix/store/644s1aciw6c5mnqsy4l8ls5sca79iin5-python2.7-Shapely-1.5.15/lib/python2.7/site-packages/shapely/_buildcfg.py:                         fallbacks=['libgeos_c.so.1', 'libgeos_c.so'])
/nix/store/644s1aciw6c5mnqsy4l8ls5sca79iin5-python2.7-Shapely-1.5.15/lib/python2.7/site-packages/shapely/_buildcfg.py:                         fallbacks=['libgeos_c.so.1', 'libgeos_c.so'])

Further testing shows the cartopy library is working (or subtly broken, depending on whether or not I'm exercising the correct path...). So I guess the "Failed" is just sort of a warning, as it's falling back on the correct path:
>>> import shapely
>>> shapely.geos._lgeos._name
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute 'geos'
>>> import shapely.geometry
Failed `CDLL(libgeos_c.so.1)`
>>> shapely.geos._lgeos._name
'/nix/store/6ln1621k5rmnvc92dmcyf18614py37va-geos-3.6.1/lib/libgeos_c.so'
>>>

Ah yes, now I see. Shapely has a function load_dll which loads a shared library. It first tries to find the library name, and then it checks the list of fallback names. Looking at the patch we have in Nixpkgs we pass the full library via the list of fallback names. That's why it fails initially but can be found afterall.
So, that means your issue is solved so now I can close this :)

Yes, thank you :-)
On 14 April 2017 at 18:55, Frederik Rietdijk<EMAIL_ADDRESS>wrote:

Ah yes, now I see. Shapely has a function load_dll which loads a shared
library. It first tries to find the library name, and then it checks the
list of fallback names. Looking at the patch we have in Nixpkgs we pass the
full library via the list of fallback names. That's why it fails initially
but can be found afterall.
So, that means your issue is solved so now I can close this :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/NixOS/nixpkgs/issues/24805#issuecomment-294202170,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACybSjzori7zGPzsLb8M9gnVUV2MhXtYks5rv7L1gaJpZM4M5VYU
.


about the rasterizer and dense mesh texture

congrats, its a great work!
Just spend some time installing pytorch3D on Windows, it's really painful, frome user friendly pespective, just FYI, it would be much easier to use nvdiffrast instead pytorch3D, since you already use nvdiffrast in your project, Also, if you have time ,pls consider replace the segment_face from tensorflow to pytorch (since you already use pytorch used in your project) .
Those two improvement coulds help others to build applications and futher research projects based on HRN.
BTW, I spend a day to read you code and add the uv data to dense mesh, its also very important for many users.

congrats, its a great work! Just spend some time installing pytorch3D on Windows, it's really painful, frome user friendly pespective, just FYI, it would be much easier to use nvdiffrast instead pytorch3D, since you already use nvdiffrast in your project, Also, if you have time ,pls consider replace the segment_face from tensorflow to pytorch (since you already use pytorch used in your project) . Those two improvement coulds help others to build applications and futher research projects based on HRN.
BTW, I spend a day to read you code and add the uv data to dense mesh, its also very important for many users.

thanks for your information. Could you share the changed code?

congrats, its a great work! Just spend some time installing pytorch3D on Windows, it's really painful, frome user friendly pespective, just FYI, it would be much easier to use nvdiffrast instead pytorch3D, since you already use nvdiffrast in your project, Also, if you have time ,pls consider replace the segment_face from tensorflow to pytorch (since you already use pytorch used in your project) . Those two improvement coulds help others to build applications and futher research projects based on HRN.
BTW, I spend a day to read you code and add the uv data to dense mesh, its also very important for many users.

Thanks for your sharing! These two problems do bring troubles to the users. We will solve these two problems and add the code for dense mesh texture in the near future.

congrats, its a great work! Just spend some time installing pytorch3D on Windows, it's really painful, frome user friendly pespective, just FYI, it would be much easier to use nvdiffrast instead pytorch3D, since you already use nvdiffrast in your project, Also, if you have time ,pls consider replace the segment_face from tensorflow to pytorch (since you already use pytorch used in your project) . Those two improvement coulds help others to build applications and futher research projects based on HRN.
BTW, I spend a day to read you code and add the uv data to dense mesh, its also very important for many users.

Hi hxflyer, I am also having trouble to generate dense uv data. Could you share the code? Many thanks!


CompareFiles could have a mode to ignore line ending differences

This is one method to fix the failing Windows tests for #497 but crucially depends on BufferedReader readLine being able to read long lines.

Related issues: #52, #152, #368


Created FSM Steering Angle Wrapper Library

Summary

Added a driver for the steering angle sensor, and set it up to publish over CAN.
Changelist

Testing Done

Tested that I can read the steering angle in the car. Testing the CAN message is pending having a working FSM.
Resolved Issues

Checklist
Please change [ ] to [x] when you are ready.

[x] I have read and followed the code conventions detailed in README.md (This will save time for both you and the reviewer!).
[x] If this pull request is longer then 500 lines, I have provided explicit justification in the summary above explaining why I cannot break this up into multiple pull requests (Small PR's are faster and less painful for everyone involved!).

Eventually got covered in #628


Create low-level API

Operations:

[ ] alter
[ ] transaction
[ ] query
[ ] mutation
[x] metrics

See:

https://dgraph.io/docs/clients/raw-http/

This is happening in dgraph.lfe right now, but it should probably go in dgraph-core.lfe ... and the high-level API should go in dgraph.


C# default values in stereokit_ui.h

Description
Lines 226 to 231 of stereokit_ui.h contain some C# default values that break the rust bindgen build.
Platform / Environment
rust binding. Linux 23.10 gnome wayland

The old signature for these function is still used so :

These default values were for C++, and default values in that format can trip up tools that parse the header as just plain C. It's wrapped in the correct macro now, so that should be good, but I'm uncertain what that error in virtual_keyboard.cpp comes from? Let me know if it still sticks around.

it works fine now. Thanks !


Deploy failed Next.js on Netlify

7:12:23 PM: Creating deploy upload records
7:12:23 PM: Failing build: Failed to prepare repo
7:12:23 PM: Failed during stage 'preparing repo': git ref refs/heads/master does not exist
7:12:23 PM: Finished processing build request in 1.153506718s

Hi @unnimona, this repo is intended for Decap CMS related issues.
For Netlify build related issues I suggest to reach out in the forums: https://answers.netlify.com/
Closing the issue, and please comment if you have any other questions.


Restored unit tests

Restored unit tests that were previously commented out due to changes introduced by the offline caching feature.

restored static method access to public

Tests passing with the following gtest flags: --gtest_shuffle --gtest_repeat=100

note: currently running but completed iteration 25 at the time of this PR

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Follow up: all 100 (local) tests passed.


Hamburger menu

Is your feature request related to a problem? Please describe.
N/A
Describe the solution you'd like
Hi Elian! Loving the theme — it's absolutely brilliant. I saw that on your personal site, you've added a hamburger menu to the mobile layout. It looks like astro-navbar, and I've tried adding that, but I'm having some difficulty with the layout. I was wondering if you wouldn't mind sharing your code for the hamburger menu implementation. Thank you!

hey @gwisk! Thanks for opening an issue!
The reason I didn't add one in the Brutal theme itself, is to don't force a framework choice upon the users. In my own website I use Astro's Vue integration. Here's a link to the component: https://github.com/ElianCodes/ElianCodes-frontend/blob/main/src/components/layout/MobileNavigation.vue
Hope this helps!

Thank you, Elian! I appreciate the help, and thanks again for the great theme.