Closed Bug 390072 Opened 17 years ago Closed 17 years ago

[SoC] Scripting Dictionary and Build System

Categories

(Camino Graveyard :: OS Integration, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.6

People

(Reporter: peeja, Assigned: peeja)

References

Details

(Keywords: fixed1.8.1.6)

Attachments

(2 files, 9 obsolete files)

47.04 KB, patch
Details | Diff | Splinter Review
50.63 KB, patch
Details | Diff | Splinter Review
Attached patch Branch version (obsolete) — Splinter Review
These patches will add Camino.sdef, which defines the scripting dictionary, to the tree. They will also update the build system to build a scriptable Camino: Branch: Adds a Run Script build phase which compiles the sdef to a .r, then Rezes the .r to Resources/Camino.rsrc. This is a little ugly, but appears to be the only way to get things to work on 10.3. This is because we need to adapt some of the terms in the Standard Suite (which is common practice), but if we use a .scriptSuite/.scriptTerminology pair Cocoa Scripting registers the default version of the Standard Suite for us, which means we get two versions of it in the dictionary. AETE-resource-based dictionaries don't have this problem. Luckily, this issue will sunset at 2.0 when we go 10.4+-only. Trunk: As of Tiger we can use an sdef directly. This version of the patch adds Camino.sdef to the Copy Resources phase and adds a key to the Info.plist pointing to it.
Attachment #274388 - Flags: review?(sfraser_bugs)
Attached patch Trunk version (obsolete) — Splinter Review
Trunk version. Simon, I've targeted you since you probably know must about these things. Please let me know if you're short on time or I should retarget for any other reason. Thanks. :)
Attachment #274390 - Flags: review?(sfraser_bugs)
Attachment #274388 - Flags: review?(sfraser_bugs) → review+
Attachment #274390 - Flags: review?(sfraser_bugs) → review+
Comment on attachment 274388 [details] [diff] [review] Branch version Thanks, Simon. Chris?
Attachment #274388 - Flags: review?(clouserw)
Attachment #274390 - Flags: review?(clouserw)
Comment on attachment 274388 [details] [diff] [review] Branch version Er, retargeting to the right cl. Sorry clouserw@gmail.com... :)
Attachment #274388 - Flags: review?(clouserw) → review?(cl-bugs)
Attachment #274390 - Flags: review?(clouserw) → review?(cl-bugs)
Attached patch Branch version, take 2 (obsolete) — Splinter Review
Fixing a couple things Smokey pointed out: * Camino.sdef didn't end with a newline. * Branch version project file called itself "CaminoBranch" in one place, due to the way I was juggling two versions of the project file.
Attachment #274388 - Attachment is obsolete: true
Attachment #274448 - Flags: review?(cl-bugs)
Attachment #274388 - Flags: review?(cl-bugs)
Attached patch Trunk version, take 2 (obsolete) — Splinter Review
Attachment #274390 - Attachment is obsolete: true
Attachment #274449 - Flags: review?(cl-bugs)
Attachment #274390 - Flags: review?(cl-bugs)
Oy. The branch version was an edited trunk project file, and those changes won't merge properly. Here's a patch that will actually make the branch changes to the branch version. Due to my working directory setup, I couldn't add the sdef in the same patch; part 2 will be just adding the sdef.
Attachment #274448 - Attachment is obsolete: true
Attachment #274471 - Flags: review?(cl-bugs)
Attachment #274448 - Flags: review?(cl-bugs)
Attachment #274472 - Flags: review?(cl-bugs)
Attachment #274449 - Flags: review?(cl-bugs) → review?(stuart.morgan)
Attachment #274471 - Flags: review?(cl-bugs) → review?(stuart.morgan)
Attachment #274472 - Flags: review?(cl-bugs) → review?(stuart.morgan)
Attachment #274472 - Attachment description: Correct branch version, part 2 (project file) → Correct branch version, part 2 (sdef file)
Attachment #274472 - Flags: superreview+
Comment on attachment 274449 [details] [diff] [review] Trunk version, take 2 sr=pink
Attachment #274449 - Flags: superreview+
Stuart: Pink wondered if you wanted to look over the sdef before it's checked in. Otherwise, this is ready to land.
Blocks: 382493
Attachment #274449 - Flags: review?(stuart.morgan) → review+
Attachment #274472 - Flags: review?(stuart.morgan) → review+
Comment on attachment 274471 [details] [diff] [review] Correct branch version, part 1 (project file) There are some issues with the branch res script: > # Complies the .scriptSuite and .scriptTerminology file from the sdef using sdp. Needs updating, since that's not what it does anymore. > sdef_path = "#{ENV['SRCROOT']}/resources/application/Camino.sdef" You may as well check for this file's existence explicitly before trying to feed it to sdp > # The output directory > output_dir = "#{ENV['DERIVED_FILES_DIR']}/aete" I'd like to see a sanity check that the derived files environment variable is set, since otherwise the script will run around the drive root. > version = "10.3" According to sdp's man page, -V isn't necessary for the resource format. > exit 1 unless system "mkdir -p #{output_dir}" Use ruby's mkdir_p > exit 1 unless system "touch #{output_dir}/foobar" # rm gets grumpy if it can't delete something. > exit 1 unless system "rm #{output_dir}/*" At the very least the argument needs to be quoted, as the path may have spaces, but it would be much safer to use Dir.glob to find any .r files and then use ruby's unlink on each of them, bypassing the shell entirely. > exit 1 unless system "/Developer/Tools/Rez #{ENV['DERIVED_FILES_DIR']}/aete/* ... Again, at the very least the arguments need to be quoted, but I'd much rather you find the path to the .r file and then use the multi-argument version of |system| so you aren't going through the shell.
Attachment #274471 - Flags: review?(stuart.morgan) → review-
(I'll try to get the trunk stuff landed tomorrow, BTW.)
Attached file New "Compile Scripting Files" Script (obsolete) —
New version of the sdp/Rez script. I discovered that you can, in fact, have sdp output to a specific file, not just a directory. (That gets messy if you're outputting multiple files, but now we're just making the one .r file.) So: 1. Comment at the top fixed. 2. Existence of Sdef checked. 3. DERIVED_FILES_DIR checked. (Fails if not set. Could provide a fallback, but that should be unnecessary.) 4. -output_dir = "#{ENV['DERIVED_FILES_DIR']}/aete" +output_file = "#{ENV['DERIVED_FILES_DIR']}/CaminoScripting.r" 5. -V argument removed. 6. System calls dealing with 'aete' directory removed, since we don't even use it anymore. 7. Rez command is passed arguments directly, not through a shell. 8. Rez command is not exec'd rather than system'd so that Rez can provide error reporting directly to Xcode. Before, we simply exited 1 if Rez exited nonzero. Stuart, I know you're manually applying a bunch of project changes, so I'm posting this as a Ruby script rather than a project patch. The contents can be pasted over the old script in the project.
Attachment #275328 - Flags: review?(stuart.morgan)
Comment on attachment 275328 [details] New "Compile Scripting Files" Script r=me. There's still a small possibility of issues with the sdp command, if someone has a path with a |'| in it, but given that that's unlikely, the command isn't particularly dangerous, and this is code that will all go away in the foreseeable future, I'm fine with leaving that until it actually causes a problem for someone.
Attachment #275328 - Flags: review?(stuart.morgan) → review+
Comment on attachment 275328 [details] New "Compile Scripting Files" Script I was a bit too quick there; per IRC discussion, this needs another spin to mkdir_p the output directory, or the build will fail.
Attachment #275328 - Flags: review+ → review-
Combined patch for this and bug 385989, as landed on trunk. Leaving open for branch.
Attachment #274449 - Attachment is obsolete: true
As Stuart said, the DERIVED_FILES_DIR may not exist when the script is run. The script now ensures that the output directory for the .r ($DERIVED_FILES_DIR) exists. Also moved the declaration of output_file out of the conditional.
Attachment #275328 - Attachment is obsolete: true
Attachment #275369 - Flags: review?(stuart.morgan)
Comment on attachment 275369 [details] Shell script, now mkdir_p'ing the DERIVED_FILES_DIR It turns out aete is only used for SE display in Cocoa; we need the .rsrc for SE cleanness, and the .script* stuff for making it actally work.
Attachment #275369 - Flags: review?(stuart.morgan) → review-
This one looks like it's building right, but Script Editor's not even opening a dictionary (or giving an error) when I test. Hoping it's local, since the build looks right on inspection. Stuart, have at it. :)
Attachment #275369 - Attachment is obsolete: true
Attachment #275523 - Flags: review?(stuart.morgan)
Comment on attachment 275523 [details] Sdp script: Now builds .rsrc & .script* Builds look good this time, r=me for hopefully the last time ;) I'll try to get this landed tonight if I have time.
Attachment #275523 - Flags: review?(stuart.morgan) → review+
Combined patch for this and bug 385989, as landed on MOZILLA_1_8_BRANCH.
Attachment #274471 - Attachment is obsolete: true
Attachment #274472 - Attachment is obsolete: true
Attachment #275523 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.1.6
Resolution: --- → FIXED
Target Milestone: --- → Camino1.6
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: