New package develop mode and lock file support. #913

bobeff · 2021-04-24T20:47:50Z

This pull request contains a new package develop mode and lock files support. It depends on this pull request in Nim. The current minimal supported version of Git is bumped to 2.22 from 2019-06-07.

In order to avoid repetition see the following sections from the documentation for more details:

This pull request contains also a significant amount of code refactoring. One of the big refactoring changes is splitting of tester.nim in modules by suites. You could look at the very detailed commit messages for more particulars.

bobeff · 2021-04-24T21:01:06Z

I'm not sure how to specify the link in .github/workflows/test.yml to the supported Nim version. Commit from this pull request in Nim is required for a successful build. Could someone help with this?

narimiran

Here are some minor suggestions/fixes for the readme.

(The rest will be reviewed later)

readme.markdown

dom96 · 2021-06-27T19:42:19Z

Just attempted to compile this on Nim 1.4.8 and got:

nimble/src/nimblepkg/sha1hashes.nim(39, 10) Error: undeclared identifier: 'isValidSha1Hash'

Is this in a Nim fork of yours?

bobeff · 2021-06-27T19:46:48Z

Just attempted to compile this on Nim 1.4.8 and got:
nimble/src/nimblepkg/sha1hashes.nim(39, 10) Error: undeclared identifier: 'isValidSha1Hash'
Is this in a Nim fork of yours?

Yes. You need the changes from the nimble-lockfile-support branch to compile it.

dom96 · 2021-07-01T13:14:50Z

@bobeff if you get a chance today, please rebase. Me and @Araq plan to review tomorrow.

bobeff · 2021-07-02T00:33:48Z

Sorry but the pull request is not ready for merge. After introducing parallel package downloads via the asynctools library now the Nimble is crashing on Windows. Also, I have to implement two additional features for which @zah specifically insisted.

nimble develop --with-dependencies - Option to put in a develop mode also the dependencies of the develop command packages.
nimble develop --develop-file=<file_name> - Option to save the develop mode dependencies in arbitary develop file.

I also have to update the documentation.

src/nimblepkg/version.nim

src/nimblepkg/packageparser.nim

src/nimblepkg/tools.nim

dom96 · 2021-07-02T10:27:44Z

src/nimblepkg/checksums.nim

+  while true:
+    var bytesRead = readChars(file, buffer)
+    if bytesRead == 0: break
+    checksum.update(buffer[0..<bytesRead])


What is the alternative?

Well, I made it with the toOpenArray procedure to avoid the copy.

src/nimblepkg/displaymessages.nim

src/nimblepkg/download.nim

src/nimblepkg/jsonhelpers.nim

dom96 · 2021-07-02T13:10:17Z

src/nimblepkg/packageparser.nim

@@ -349,7 +345,7 @@ proc readPackageInfo(result: var PackageInfo, nf: NimbleFile, options: Options,
    return

  result = initPackageInfo(nf)
-  let minimalInfo = getNameVersion(nf)
+  result.isLink = not nf.startsWith(options.getPkgsDir)


This looks flaky

Any suggestions on how to avoid it?

src/nimblepkg/paths.nim

src/nimblepkg/sha1hashes.nim

dom96 · 2021-07-02T16:09:43Z

For reference, what we posted in Telegram.

So here are our top-level asks:

develop - too many flags (unclear why they are necessary)
sync files (too much magic, again unclear why they are necessary)
asyncproc - use osproc.execProcesses
git downloads by default instead of via tarballs (consider to remove this feature)
for future PRs, keep them small and communicate with us what the plans are!

These are our main tasks for you to resolve, we can change the rest of the details ourselves.

On a positive note, we really like the setup implementation and it is cool to see lockfiles working!

bobeff · 2021-07-04T02:16:28Z

develop - too many flags (unclear why they are necessary)

There is a description of what every flag does. They are needed for manipulating the nimble.develop file. Otherwise, the manipulation should be done by hand directly opening and editing the JSON file.

sync files (too much magic, again unclear why they are necessary)

Sync files are necessary for implementing the develop packages repositories synchronization functionality. Look at the workingCopyNeeds procedure in the developfile.nim for details on how the information from them is being used.

asyncproc - use osproc.execProcesses

Zah, told me to use the asynctools library and async/await for implementing parallel downloads. I don't understand how osproc.execProcess helps. Is it also asynchronous? I thought that there are no asynchronous processes in the Nim standard library.

git downloads by default instead of via tarballs (consider to remove this feature)

I was specifically asked by Zah to add this feature and to be enabled by default. I think that this is nice optimization because tarball downloads are faster.

for future PRs, keep them small and communicate with us what the plans are!

Well, I think that the lock files, the new develop mode, the tarball downloads, and the parallel downloads are 4 distinct features and it was possible to implement them as 4 independent pull requests, but I was specifically asked by Zah to implement all of them with the lock files. The functionalities now are too intertwined and it will be a nightmare if I try to separate them.

haxscramper · 2021-07-04T10:57:38Z

New RFC for nimble development roadmap nim-lang/RFCs#398 talks about some features like nimble setup (that implemented in this PR, but don's seem to be discussed in any other RFC). It would be really nice if you could write about what are other features that you added and how they should affect day-to-day nimble uses, in order to provide a full overview of the upcoming changes to those who are not familiar with this (quite extensive) PR

So far the feature list seems quite large - far beyond of what original specification and includes things like

Parallel package download
Intermediate package.nim.cfg file
Lock files
Changes to the nimble develop command
Changes to nimble check

bobeff · 2021-07-05T11:59:24Z

asyncproc - use osproc.execProcesses

By using the asynctools library it is possible to implement some parallelization of downloads when doing recursive downloads without using a lock file. It is not possible with osproc.execProcesses.

dom96 · 2021-07-05T13:06:57Z

@bobeff I think you can use osproc.execProcesses to execute multiple processes in parallel, these can perform the download in parallel. Asynctools is the same but it uses async to accomplish it.

bobeff · 2021-07-05T13:12:04Z

@dom96 Yes, I understood this but if in the future if we try to implement parallel downloads when not using a lock file it will be not possible for the recursive algorithm without async.

All tuples from the same type which are found in more than one place in the code are type aliased and their usage is replaced with the corresponding alias. Related to nim-lang#127

Related to nim-lang#127

- The package sha1 checksum is added to the `PackageInfo` object when it is being initialized either by calculating it or by getting it from the package directory name in the local package cache. - When package is being written in the local cache its sha1 checksum is added to the directory name alongside its name and version. - Reverse dependencies format is changed to contain also the package sha1 checksum. Conversion between the old and the new formats is implemented for backward compatibility. All functionality related to the reverse dependencies is fixed to work properly with the new format. - The indexing of the `nimbledata.json` fields is changed to use values from an enum for their names to avoid typing mistakes. - All unit tests are fixed to pass with the new changes and new unit tests are added where it is needed. Related to nim-lang#127

Fix `processDeps` procedure to not return a dependency multiple times when it is a common dependency of more than one package. Related to nim-lang#127

The `cd` template from `tools.nim` did not restore the previous directory in the case when return from the function is made in the body. The defer statement is used to fix this. Related to nim-lang#127

The parallel package downloads are removed, because the `asynctools` library for asynchronous processes, that is used is not production-ready. It has several hard-to-find bugs causing а crashing of Nimble. First, they must be fixed before using it. Related to nim-lang#127

bobeff · 2021-08-11T12:22:35Z

I removed the parallel downloads feature from the pull request, because the asynctools library is not stable and its usage causes a crashing of Nimble. The rewrite of the code with the osproc.execProcesses cannot be done easily and it will require significant code rearrangement. Moreover, it will not allow asynchronous downloads of not-locked dependencies, and for that reason, it is not a direction in which I want to move. I hope that in the future the asynctools library will be fixed and this will enable us to properly implement parallel downloads for both locked and free dependencies.

bobeff · 2021-08-11T12:42:17Z

The CI gives a 403 rate limit exceeded error. This seems like something caused by too many requests to GitHub during the tests execution and not a problem with Nimble itself.

dom96 · 2021-08-22T22:37:51Z

The CI gives a 403 rate limit exceeded error. This seems like something caused by too many requests to GitHub during the tests execution and not a problem with Nimble itself.

We didn't see similar problems when cloning via git was the default. Sounds like pretty good evidence that we should revert this change.

Downloading of Nimble packages as tarballs when working with GitHub is now disabled by default. The option `--no-tarballs` for disabling it is removed and a new option `--tarballs` for explicitly enabling the feature is added instead. Related to nim-lang#127

Pass the requested branch to the `git` or `hg` command instead of the "branch" word. Related to nim-lang#127

bobeff · 2021-08-23T17:50:27Z

The CI gives a 403 rate limit exceeded error. This seems like something caused by too many requests to GitHub during the tests execution and not a problem with Nimble itself.

We didn't see similar problems when cloning via git was the default. Sounds like pretty good evidence that we should revert this change.

I made the tarballs download feature disabled by default. But now a problem with calculating checksums of symlink files in the repository has appeared under Windows. I will investigate it tomorrow.

Sometimes when downloading a package some symbolic links files cannot be open on Windows when Nimble is run as administrator. For this reason, add exception handling to the `open` procedure to avoid the crash and display a warning message that the file content will not be count in the calculation of the package's checksum. Related to nim-lang#127

src/nimblepkg/checksums.nim

narimiran · 2021-09-07T12:48:09Z

https://forum.nim-lang.org/t/8404#54193

I have experienced the same message/error.

follow up #19821 dights cannot clash with letters 'Z' and 'O' For `threading-0.2.0-288108d1dfa34d5ade5ce4d922af51909c83cebf` Before: raiseNilAccess__OOZOOZOnimbleZpkgs50Zthreading4548O50O4845505656494856d49dfa5152d53ade53ce52d575050af5349574857c5651cebfZthreadingZsmartptrs_u4 After: raiseNilAccess__OOZOOZOnimbleZpkgs2Zthreading450O2O045288108d1dfa34d5ade5ce4d922af51909c83cebfZthreadingZsmartptrs_u4 <del> nimble or something might use `git rev-parse --short HEAD` to shorten the length of package version names ref nim-lang/nimble#913 </del>

bobeff force-pushed the feature/lock-file branch from 7a9b4ac to 140b858 Compare April 24, 2021 20:59

bobeff force-pushed the feature/lock-file branch 5 times, most recently from dd83f14 to ebdb777 Compare April 29, 2021 11:37

narimiran reviewed Jun 1, 2021

View reviewed changes

bobeff force-pushed the feature/lock-file branch from 2794d67 to 7b1fde6 Compare June 1, 2021 19:08

bobeff closed this Jul 2, 2021

Araq reopened this Jul 2, 2021

bobeff marked this pull request as draft July 2, 2021 10:44

dom96 requested changes Jul 2, 2021

View reviewed changes

bobeff force-pushed the feature/lock-file branch from d7cf5b5 to a433091 Compare July 4, 2021 01:14

dom96 mentioned this pull request Jul 4, 2021

Nimble improvements nim-lang/RFCs#398

Open

bobeff added 5 commits July 8, 2021 00:47

Add type aliases for some tuples

4fb4e2c

All tuples from the same type which are found in more than one place in the code are type aliased and their usage is replaced with the corresponding alias. Related to nim-lang#127

Implement calculation of a package sha1 checksum

b72fdc4

Related to nim-lang#127

Deduplicate packages returned by processDeps proc

09b1368

Fix `processDeps` procedure to not return a dependency multiple times when it is a common dependency of more than one package. Related to nim-lang#127

Fix cd template to restore the previous dir

a07ea8e

The `cd` template from `tools.nim` did not restore the previous directory in the case when return from the function is made in the body. The defer statement is used to fix this. Related to nim-lang#127

bobeff closed this Aug 11, 2021