Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature] Interlize chmod as Conan tool #8003

Open
1 task done
uilianries opened this issue Nov 4, 2020 · 8 comments
Open
1 task done

[feature] Interlize chmod as Conan tool #8003

uilianries opened this issue Nov 4, 2020 · 8 comments

Comments

@uilianries
Copy link
Member

uilianries commented Nov 4, 2020

Hi!

As you may know, chmod is a nice Unix tool, which is used to handle file permission.

On Python, we have os.chmod, but it's limited in terms of solution on Windows (see #7966).

Doing a quick search on CCI we have os.chmod present there, but there is no pattern for its usage, sometimes we force the permission number, sometimes we just update (add/remove) what we need.

My suggestion is adding a wrapper for chmod:

tools.chmod(os.path.join(self.package_folder, "bin", "foo"), stat.S_IEXEC)

Or

tools.chmod_x(os.path.join(self.package_folder, "bin", "foo"))
tools.chmod_w(os.path.join(self.package_folder, "bin", "foo"))
tools.chmod_r(os.path.join(self.package_folder, "bin", "foo"))

Or

tools.chmod(os.path.join(self.package_folder, "bin", "foo"), read=True, write=False, exec=True)

A plus for the third option: Windows users/developers may not know about numerical permission.

Also, as os.chmod is limited for Windows, we could include the project oschmod, which is able to fix the permission on Windows. Thus, tools.chmod would be effective for any OS. There is a nice blog post about oschmod on Medium.

/cc @a4z @solvingj

@solvingj
Copy link
Contributor

solvingj commented Nov 4, 2020

I've wanted this kind of tool for my conan recipes in the past. I don't know enough about the various scenarios which require all the parameters of os.chmod to know if there's a "general case" for Conan to add a tool with sensible defaults. I hope so. There probably is, since we collapsed most of subprocess down to self.run, but I imagine some people will find reasons to disagree that this is in scope for Conan. Will be interesting to see.

@memsharded
Copy link
Member

https://github.com/YakDriver/oschmod has 3 stars in Github, I wouldn't rely on this, version 0.3.12 in PyPI. I wouldn't rely on this yet. Plus I would like to know real cases in ConanCenter that requires this level of permission modification for Windows.

This are the current occurrences in ConanCenter

recipes\7zip\19.00\conanfile.py:
  65              fn = os.path.join("CPP", "Build.mak")
  66:             os.chmod(fn, 0o644)
  67              tools.replace_in_file(fn, "-MT", "-{}".format(str(self.settings.compiler.runtime)))

recipes\cunit\all\conanfile.py:
  63              for f in glob.glob("*.c"):
  64:                 os.chmod(f, 0o644)
  65  

recipes\depot_tools\all\conanfile.py:
  50          def chmod_plus_x(name):
  51:             os.chmod(name, os.stat(name).st_mode | 0o111)
  52  

recipes\freetype\all\conanfile.py:
  133          if os.name == "posix":
  134:             os.chmod(filename, os.stat(filename).st_mode | 0o111)
  135  

recipes\gmp\all\conanfile.py:
  53                  configure_stats = os.stat(configure_file)
  54:                 os.chmod(configure_file, configure_stats.st_mode | stat.S_IEXEC)
  55              configure_args = []

recipes\libdb\all\conanfile.py:
  186                          binpath = os.path.join(bindir, fn)
  187:                         os.chmod(binpath, 0o755)  # Fixes PermissionError(errno.EACCES) on mingw
  188                          os.remove(binpath)

recipes\meson\all\conanfile.py:
  42          if os.name == 'posix':
  43:             os.chmod(filename, os.stat(filename).st_mode | 0o111)
  44  

recipes\ninja\1.9.x\conanfile.py:
  55              name = os.path.join(self.package_folder, "bin", "ninja")
  56:             os.chmod(name, os.stat(name).st_mode | 0o111)
  57          self.env_info.PATH.append(os.path.join(self.package_folder, "bin"))

recipes\nspr\all\conanfile.py:
  146              for f in os.listdir(os.path.join(self.package_folder, "lib")):
  147:                 os.chmod(os.path.join(self.package_folder, "lib", f), 0o644)
  148  

recipes\openssl\1.x.x\conanfile.py:
  705              with tools.chdir(os.path.join(self.package_folder, "lib")):
  706:                 os.chmod("libssl.so.1.0.0", 0o755)
  707:                 os.chmod("libcrypto.so.1.0.0", 0o755)
  708  

recipes\waf\all\conanfile.py:
  45  
  46:         os.chmod(os.path.join(binpath, "waf"), 0o755)
  47:         os.chmod(os.path.join(binpath, "waf-light"), 0o755)
  48  

recipes\zlib\1.2.11\conanfile.py:
  37              st = os.stat(configure_file)
  38:             os.chmod(configure_file, st.st_mode | stat.S_IEXEC)
  39  

recipes\zlib\1.2.8\conanfile.py:
  45              st = os.stat(configure_file)
  46:             os.chmod(configure_file, st.st_mode | stat.S_IEXEC)
  47  

It seems a wrapper that would simplify a bit the UI and avoid the st.st_mode | stat.S_IEXEC could be nice.

@uilianries
Copy link
Member Author

YakDriver/oschmod has 3 stars in Github, I wouldn't rely on this, version 0.3.12 in PyPI. I wouldn't rely on this yet.

We could internalize its code.

Plus I would like to know real cases in ConanCenter that requires this level of permission modification for Windows.

The chmod has no effect on windows, we would need subprocess as @solvingj commented, but it would be nice if we could use API call instead, safer and faster than a fork. I can see few cases, most when we need to add exec permission for scripts (.sh, .py, ...) on Windows. I think any .exe file will be executable by default on Windows.

@memsharded
Copy link
Member

The chmod has no effect on windows, we would need subprocess as @solvingj commented, but it would be nice if we could use API call instead, safer and faster than a fork. I can see few cases, most when we need to add exec permission for scripts (.sh, .py, ...) on Windows. I think any .exe file will be executable by default on Windows.

I mean, what is the real need. What library or Conan package requires this level of changing permissions. What cannot be built in ConanCenter unless using this code. It doesn't matter much if it doesn't have effect on Windows as long as libraries link and executables run.

@uilianries
Copy link
Member Author

I mean, what is the real need. What library or Conan package requires this level of changing permissions. What cannot be built in ConanCenter unless using this code. It doesn't matter much if it doesn't have effect on Windows as long as libraries link and executables run.

Maybe not so focused on ConanCenter, but maybe for companies which are packaging internal packages.

We have a thread on Conan Slack channel: https://cpplang.slack.com/archives/C41CWV9HA/p1604505594005800?thread_ts=1604505490.005700&cid=C41CWV9HA /cc @a4z

@memsharded
Copy link
Member

Maybe not so focused on ConanCenter, but maybe for companies which are packaging internal packages.

It is not clear from that thread what is the companies use case either. I think it is fine to add a Conan tool to abstract away the details of os.chmod a bit, but so far I don't see enough evidence to add or to depend on YakDriver/oschmod into our codebase.

@a4z
Copy link
Contributor

a4z commented Nov 4, 2020

as a package creator, when needing to be sure an executable has execution permission, it would be nice to have something like tools.make_executable(filename) and not be forced to add repetitive boiler plate, and maybe .

This will improve the readability dramatically, since today various names and solutions for the same thing are used
but it will also improve the writing and authoring experience of packages, and welcome new recipe authors and maintainers.

About implementation details, I do not have any strong opinion on what the best way is, but it would deffinitely be nice to get rid of the if not windows then chmod (and hopefully get all the details right) and on windows no idea

@a4z
Copy link
Contributor

a4z commented Nov 4, 2020

PS: my current use case
https://github.com/cross-language-cpp/djinni-generator

the Windows .bat file is exactly the same file as the non .bat file,
some kind of dark magic that works on all platforms (Linux OSX Windows) as long as there is some java findable (btw, can we please proceed with conan-io/conan-center-index#2756 ;-)
If it is somehow possible to not be in need of having a separate name for Windows, and the normal djinni file could be called this would be even more magic.

But please note, if you find that this use case is strange or too special
the main reason for my request for a tool is still what I wrote above #8003 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants