Split upload command into separate file #9594

Zopolis4 · 2024-04-03T08:08:26Z

Split out upload command to commands/upload.rb.

Behavior changes:

Remove support for uploading without passing a package name.
- This wasn't working anyways, and I can't conceive of a situation where you are uploading packages without knowing their names or having just built them.
- If this functionality is still needed, I can write a bash harness to implement this functionality externally.
Remove support for uploading multiple architectures.
- This functionality was working, but it would have dramatically complicated the code to continue to implement.
- If this functionality is needed, just setting the ARCH environment variable should take care of things.
Remove [options] from the docopt section for crew upload.
- Same rationale as the last times I've done this.
Remove verbose argument.
- Because of the refactor, the code that previously would have used this is now gone.

Refactoring changes:

Rework load_package in lib/package.rb to only take one argument, being the path to the package file.
- Derive the package name in the function itself, rather than the weird setup we had before with a default argument value (when would we ever want to pass a different value to that?).
Rework set_package in to only take one argument, , being the path to the package file.
- All it was using the previous argument for was to unnecessarily pass it to load_package.
Rework search to accept a package name, path, and a value for silent.
- Use named parameters for the last two to save boilerplate.
- Use a default value for pkg_path, as it is a named parameter rather than just being an optional one.
Move the rubocop section that was previously in def upload to def copy_package.
- Better to sanitize the package file earlier in the process.
Load the package file from CREW_LOCAL_REPO_ROOT when passing the package object to Command.upload, as this is the package file that we will be working with.
Rework the way we look for GITLAB_TOKEN to not need a trailing if and .nil? by using a block in the fallback of ENV.fetch to abort if we can't find it.
Check no_zstd to find the extension of the newly built binaries and set the local variable binary_compression to that.
- Since we just built the binary, we can be confident in the extension, as chromebrew will only use one of two compression programs.
Use net/http instead of curl to upload the binary.
Use .sub! and .gsub! instead of sed to update the package file.

Tested and working on all architectures.

Run the following to get this pull request's changes locally for testing.

CREW_REPO=https://github.com/Zopolis4/chromebrew.git CREW_BRANCH=projectregula6 crew update

uberhacker · 2024-04-03T16:01:02Z

bin/crew

@@ -202,10 +203,9 @@ def generate_compatible
  puts 'Generating compatible packages done.'.orange if @opt_verbose
 end

-def search(pkg_name, silent = false)
-  pkg_path = File.join(CREW_PACKAGES_PATH, "#{pkg_name}.rb")
+def search(pkg_name, pkg_path: File.join(CREW_PACKAGES_PATH, "#{pkg_name}.rb"), silent: false)


Why does this need to be a parameter? Keep it simple!

Please read the reasons I gave in the PR description...

Behavior changes:

Remove support for uploading without passing a package name.
This wasn't working anyways, and I can't conceive of a situation where you are uploading packages without knowing their names or having just built them.
I'm not sure why you think this wasn't working when I've been using it for over a month now without an issue.
If this functionality is still needed, I can write a bash harness to implement this functionality externally.
No, the whole purpose of adding this function was to remove the need for bash scripts and incorporate it as a crew command instead.
Remove support for uploading multiple architectures.
This functionality was working, but it would have dramatically complicated the code to continue to implement.
It was already written and simplifies the process for contributors. Because the code is complicated is not a valid reason for removing it altogether.
If this functionality is needed, just setting the ARCH environment variable should take care of things.
This functionality was added for a reason
Remove [options] from the docopt section for crew upload.
Same rationale as the last times I've done this.
Remove verbose argument.
Because of the refactor, the code that previously would have used this is now gone.
Then add it back. We'll want to control verbosity.
Refactoring changes:

Rework load_package in lib/package.rb to only take one argument, being the path to the package file.
How about rework to take one argument of the package name instead?
Derive the package name in the function itself, rather than the weird setup we had before with a default argument value (when would we ever want to pass a different value to that?).
Rework set_package in to only take one argument, , being the path to the package file.
All it was using the previous argument for was to unnecessarily pass it to load_package.
Nothing weird about passing the package name and having the function determine the path.
Rework search to accept a package name, path, and a value for silent.
No need for a path. Search should ONLY look for crew packages, not your local repo directory.
Use named parameters for the last two to save boilerplate.
Use a default value for pkg_path, as it is a named parameter rather than just being an optional one.
Again, package name makes more sense than package path.
Move the rubocop section that was previously in def upload to def copy_package.
Better to sanitize the package file earlier in the process.
Load the package file from CREW_LOCAL_REPO_ROOT when passing the package object to Command.upload, as this is the package file that we will be working with.
No need to do this at all.
Rework the way we look for GITLAB_TOKEN to not need a trailing if and .nil? by using a block in the fallback of ENV.fetch to abort if we can't find it.
Check no_zstd to find the extension of the newly built binaries and set the local variable binary_compression to that.
Since we just built the binary, we can be confident in the extension, as chromebrew will only use one of two compression programs.
Use net/http instead of curl to upload the binary.
Use .sub! and .gsub! instead of sed to update the package file.
We're not only substituting here. We're ADDING new lines if the section doesn't exist yet.

I'm not sure why you think this wasn't working when I've been using it for over a month now without an issue.

Probably because it's been broken whenever I tried to do it.

No, the whole purpose of adding this function was to remove the need for bash scripts and incorporate it as a crew command instead.

Again, I do not understand how you can be in a scenario where you don't know the names of the binaries you are uploading. I genuinely do not see a usecase for not passing a package name.

Then add it back. We'll want to control verbosity.

Because we aren't using system commands anymore, we can't just print the curl command we are using. If it was possible to add this code back I would have.

It was already written and simplifies the process for contributors. Because the code is complicated is not a valid reason for removing it altogether.
This functionality was added for a reason

Again, what is the reason? When are you in a scenario where you aren't on the same architecture as the binaries you've built?

Again, package name makes more sense than package path
No need to do this at all.

Please see my other comment where I explained why this is not the case.

Actually, I can probably re-implement some of the verbosity, I'll do that when I've got the time.

Added back the verbosity that was practical to do.

uberhacker · 2024-04-03T16:04:23Z

bin/crew

  args['<name>'].each do |name|
-    search name
-    upload name if @pkg.name
+    search name, pkg_path: File.join(CREW_LOCAL_REPO_ROOT, 'packages', "#{name}.rb")


No need to pass the package path. Let the function figure it out from the predefined constant. Also, we do not want to use CREW_LOCAL_REPO_ROOT. We should be using CREW_PACKAGES_PATH.

We want to use CREW_LOCAL_REPO_ROOT-- that is the location of the package file we will be editing once we have uploaded, so we want to get the values from that file.

You have to copy the package from your local repo to the crew packages directory BEFORE you can even test it so I'm not sure why you think this needs a refactor. Also, please stop resolving conversations.

git clone https://github.com/chromebrew/chromebrew.git cd chromebrew crew build packages/libnvme.rb

The package file never makes it to CREW_PACKAGES_PATH.

This is NOT my workflow. This is harder than a simple crew build libnvme and NOT requiring the path. The path is abstracted which is the whole point of why I wrote this originally.

I'm abstracting the path so if you are making changes to the package in your local repo, crew will detect that it differs from the crew repo package and offer to copy it over so it can run it inside crew. So simply running crew build/install/reinstall <package> will enable you to test the package without needed to copy the package file over manually. My vision is for contributors to be able to do everything they need to do to create and update packages inside their own forked repo instead of working inside crew directly. This way changes are easier to manage and removes the potential human error element from the process. I know @satmandu has a different workflow and that is fine but I still think mine is easier. :)

So your goal is that the user does all the changes in their local repository, but crew then copies it to the installation, works with that, then copies back any changes to the local repository?

I'll point out that the workflow I am enabling here does not require any manual copying, and in fact just removes the requirement for a package file to be inside the crew installation. (I'm also trying to reach an agreement on this because it will significantly affect how I refactor crew check, which I'd like to get started on soon)

I still don't think you fully understand my workflow. The way it currently works (by design) is any change you make in your local working repo will be prompted to copy to crew for installation (no manual copying). I don't think a refactor is necessary because it will over complicate the process and the package has to end up in crew anyway to test the install. You can always do a crew update to restore back to the original version if you are updating existing packages.

The point of these changes to search is to ensure that there is no need for any copying, whether automatic or manual. I'd argue that this simplifies the process, in fact.

uberhacker

I'm of the opinion we should only pass the package name to any function. The function itself should figure out the path. We also need to be consistent with the calls. Right now, some functions require the path and others just the package name. Let's just send the package name only.

commands/upload.rb

commands/help.rb

Zopolis4 · 2024-04-03T20:14:00Z

I'm not entirely sure what you mean by this-- we only pass the package path to search, which loads the package file.

uberhacker · 2024-04-03T20:31:23Z

commands/upload.rb

+    file.sub!(/binary_compression.*/, "binary_compression '#{binary_compression}'")
+
+    # Update the package file with the new binary_sha256 value.
+    file.gsub!(pkg.binary_sha256[ARCH.to_sym], Digest::SHA256.hexdigest(File.read(pkg_tarfile_path)))


This handles updating the package if the sha256sum hash exists but what about new packages that don't have the section added yet?

New packages will need a dummy section something like this one:

binary_sha256({ aarch64: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', armv7l: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', i686: 'iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii', x86_64: 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx' })

All the code to not require this is already there. You are digressing the code and making the process harder instead of making it better.

All the code to not require this is already there.

This code is broken, at least in my experience.

You are digressing the code and making the process harder instead of making it better.

Would you be open to the addition of something like a crew create command which creates a package template file? My intention is not to make things more difficult, and I'd be happy to implement this functionality elsewhere where it is more suited.

Sure, that sounds like a great idea. Maybe provide a few basic prompts like the package name, description, version, etc. It would be nice to search through all the packages to determine if there is a name conflict also.

Sounds good! I'll mark this PR as draft until then because I don't want to remove this functionality until an option is available.

uberhacker · 2024-05-22T17:11:45Z

lib/const.rb

@@ -364,7 +364,7 @@
    crew test [-v|--verbose] [<name> ...]
    crew update [options] [-v|--verbose] [<compatible>]
    crew upgrade [options] [-k|--keep] [-s|--source] [-v|--verbose] [<name> ...]
-    crew upload [options] [-v|--verbose] [<name> ...]
+    crew upload <name> ...


I think we should keep the -v option for debugging purposes.

The old verbose option just printed the system commands being used-- no system commands are used anymore, so I wasn't sure how to re-implement it.

Zopolis4 added this to the bin/crew refactoring milestone Apr 3, 2024

Zopolis4 requested review from uberhacker, satmandu and supechicken April 3, 2024 08:08

Zopolis4 force-pushed the projectregula6 branch 3 times, most recently from f568908 to 3e99fc0 Compare April 3, 2024 08:28

uberhacker reviewed Apr 3, 2024

View reviewed changes

uberhacker requested changes Apr 3, 2024

View reviewed changes

uberhacker reviewed Apr 3, 2024

View reviewed changes

commands/upload.rb Show resolved Hide resolved

uberhacker reviewed Apr 3, 2024

View reviewed changes

commands/help.rb Show resolved Hide resolved

uberhacker reviewed Apr 3, 2024

View reviewed changes

Zopolis4 marked this pull request as draft April 3, 2024 21:36

Zopolis4 mentioned this pull request Apr 10, 2024

Add create command #9624

Draft

Zopolis4 marked this pull request as ready for review April 12, 2024 13:49

Zopolis4 force-pushed the projectregula6 branch 2 times, most recently from d0b799c to 5391b6f Compare April 14, 2024 10:31

Zopolis4 mentioned this pull request Apr 16, 2024

Rework load_package, set_package, search and copy_package #9658

Merged

Zopolis4 force-pushed the projectregula6 branch 2 times, most recently from 336829d to f3b134e Compare April 21, 2024 13:58

Zopolis4 force-pushed the projectregula6 branch from f3b134e to 719cebd Compare May 1, 2024 22:28

uberhacker reviewed May 22, 2024

View reviewed changes

Split upload command into separate file

47910d8

Zopolis4 force-pushed the projectregula6 branch from 719cebd to 47910d8 Compare May 22, 2024 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split upload command into separate file #9594

Split upload command into separate file #9594

Zopolis4 commented Apr 3, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

Zopolis4 Apr 3, 2024

Zopolis4 Apr 10, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

uberhacker Apr 3, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

Zopolis4 Apr 9, 2024

uberhacker Apr 10, 2024

Zopolis4 Apr 10, 2024

uberhacker left a comment

Zopolis4 commented Apr 3, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

uberhacker Apr 3, 2024

Zopolis4 Apr 3, 2024

Zopolis4 Apr 10, 2024

uberhacker May 22, 2024

Zopolis4 May 22, 2024

Split upload command into separate file #9594

Are you sure you want to change the base?

Split upload command into separate file #9594

Conversation

Zopolis4 commented Apr 3, 2024

Run the following to get this pull request's changes locally for testing.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uberhacker left a comment

Choose a reason for hiding this comment

Zopolis4 commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment